Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generator with options #111

Merged
merged 15 commits into from
Jan 23, 2023
Merged

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
generator.go Show resolved Hide resolved
generator.go Show resolved Hide resolved
generator.go Show resolved Hide resolved
@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
generator.go Outdated Show resolved Hide resolved
@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 Report

Base: 100.00% // Head: 97.93% // Decreases project coverage by -2.06% ⚠️

Coverage data is based on head (d0c4244) compared to base (e1079f3).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##            master     #111      +/-   ##
===========================================
- Coverage   100.00%   97.93%   -2.07%     
===========================================
  Files            4        4              
  Lines          411      436      +25     
===========================================
+ Hits           411      427      +16     
- Misses           0        6       +6     
- Partials         0        3       +3     
Impacted Files Coverage Δ
generator.go 95.10% <66.66%> (-4.90%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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