Skip to content

Conversation

LeonanCarvalho
Copy link
Contributor

@LeonanCarvalho LeonanCarvalho commented Dec 27, 2022

I rewrote the PR #98 authored by @mlesar based on the @theckman comments and recommendations.

Help is wanted to make it happens, this is a very desirable feature especially to generate UUIDv6 with custom timestamps.

@LeonanCarvalho LeonanCarvalho marked this pull request as ready for review December 27, 2022 14:36
@LeonanCarvalho LeonanCarvalho changed the title feat: add generator with options Generator options Dec 27, 2022
@LeonanCarvalho LeonanCarvalho requested review from baldanca and dylan-bourque and removed request for baldanca and dylan-bourque December 27, 2022 16:45
@LeonanCarvalho LeonanCarvalho changed the title Generator options Generator options (2) Dec 27, 2022
@LeonanCarvalho LeonanCarvalho requested review from dylan-bourque and baldanca and removed request for dylan-bourque and baldanca December 28, 2022 12:17
@codecov-commenter
Copy link

codecov-commenter commented Dec 30, 2022

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (6ba114c) to head (03116b5).
⚠️ Report is 92 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #111   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          448       473   +25     
=========================================
+ Hits           448       473   +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@dylan-bourque dylan-bourque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LeonanCarvalho
Copy link
Contributor Author

LeonanCarvalho commented Jan 3, 2023

@cameracker It would be great to have your review too :)

@cameracker
Copy link
Collaborator

@LeonanCarvalho thanks for the submission. What are your thoughts on the decrease of code coverage? Can we get back to 100%?

}),
)

u, err := g.NewV1()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any value in checking the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't imagine what else to check at this point. I just kept the same behaviour as the existing tests, I didn't want to modify anything pre-existing.

@cameracker cameracker mentioned this pull request Jan 6, 2023
@LeonanCarvalho
Copy link
Contributor Author

@LeonanCarvalho thanks for the submission. What are your thoughts on the decrease of code coverage? Can we get back to 100%?

I think we could include tests for nil values since there is an if with default's fallback suggested here #111 (comment)

https://app.codecov.io/gh/gofrs/uuid/pull/111

@LeonanCarvalho LeonanCarvalho changed the title Generator options (2) Generator with options Jan 9, 2023
@LeonanCarvalho
Copy link
Contributor Author

Does something flaky on those tests? I ran it on my repo and it passed
https://github.com/LeonanCarvalho/uuid/actions/runs/3788789705

@LeonanCarvalho
Copy link
Contributor Author

I found out that pkg x/tools cover is no longer active and marked as deprecated, that's why the test is falling https://pkg.go.dev/golang.org/x/tools/cmd/cover

LeonanCarvalho added a commit to LeonanCarvalho/uuid that referenced this pull request Jan 18, 2023
Recently I found that a previous change not related to workflows is failing  in my other PR
gofrs#111

It is happening because the package [golang.org/x/tools/cmd/cover](https://pkg.go.dev/golang.org/x/tools/cmd/cover) is no longer active and marked as deprecated, that's why the test is falling 


```
Run go get golang.org/x/tools/cmd/cover
  go get golang.org/x/tools/cmd/cover
  shell: /usr/bin/bash -e {0}
  env:
    GO111MODULE: auto
    GOROOT: /opt/hostedtoolcache/go/1.19.4/x64
cannot find package "golang.org/x/tools/cmd/cover" in any of:
	/opt/hostedtoolcache/go/1.19.4/x64/src/golang.org/x/tools/cmd/cover (from $GOROOT)
	/home/runner/go/src/golang.org/x/tools/cmd/cover (from $GOPATH)
Error: Process completed with exit code 1.
```
The tests are focused on 1.18 e 1.19 we will be fine
@LeonanCarvalho
Copy link
Contributor Author

LeonanCarvalho commented Jan 23, 2023

I opened another PR to solve this workflow issue #115 I would appreciate it if you guys could also take a look at it.

cameracker pushed a commit that referenced this pull request Jan 23, 2023
Recently I found that a previous change not related to workflows is failing  in my other PR
#111

It is happening because the package [golang.org/x/tools/cmd/cover](https://pkg.go.dev/golang.org/x/tools/cmd/cover) is no longer active and marked as deprecated, that's why the test is falling 


```
Run go get golang.org/x/tools/cmd/cover
  go get golang.org/x/tools/cmd/cover
  shell: /usr/bin/bash -e {0}
  env:
    GO111MODULE: auto
    GOROOT: /opt/hostedtoolcache/go/1.19.4/x64
cannot find package "golang.org/x/tools/cmd/cover" in any of:
	/opt/hostedtoolcache/go/1.19.4/x64/src/golang.org/x/tools/cmd/cover (from $GOROOT)
	/home/runner/go/src/golang.org/x/tools/cmd/cover (from $GOPATH)
Error: Process completed with exit code 1.
```
The tests are focused on 1.18 e 1.19 we will be fine
@cameracker
Copy link
Collaborator

Thanks @LeonanCarvalho !

@cameracker cameracker merged commit f1cfba7 into gofrs:master Jan 23, 2023
@LeonanCarvalho LeonanCarvalho deleted the feat/with-gen-options branch July 14, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants