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

Generate code using Go generics #20

Merged
merged 3 commits into from
Aug 10, 2022

Conversation

vincentbernat
Copy link
Contributor

The code mutation is a bit less pretty but it still works well. Tests
are kept in this case as they are easy to mutate to check that
everything is working.

However, this requires bumping the minimal Go version to 1.18.

@coveralls
Copy link

coveralls commented Jul 31, 2022

Pull Request Test Coverage Report for Build 2819622670

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 81.215%

Totals Coverage Status
Change from base Build 2819264654: 0.0%
Covered Lines: 147
Relevant Lines: 181

💛 - Coveralls

Makefile Outdated
Comment on lines 23 to 41
generics: codegen-generics
# generics -> T, except in tests
( cd generics_tree && $(SED) -i -E -e 's/\bgenerics\b/string/g' *_test.go)
( cd generics_tree && $(SED) -i -E -e 's/\bgenerics\b/T/g' *.go)
# Each defined type should be parametrized with T
( cd generics_tree && $(SED) -nE 's/^type (\w+).*/\1/p' *.go \
| while read T; do \
$(SED) -i -E -e 's/\b'$$T'\b/\0[T]/g' *.go ; \
$(SED) -i -E -e 's/\b('$$T')\[T\]/\1[string]/g' *_test.go ; \
done )
# Fix type definition
( cd generics_tree && $(SED) -i -E -e 's/^(type \w+)\[T\]/\1[T any]/' *.go)
# NewTreeVX function should be parametrized
( cd generics_tree && $(SED) -i -E -e 's/^(func NewTreeV.)/\1[T any]/' *.go)
( cd generics_tree && $(SED) -i -E -e 's/(NewTreeV.)\(/\1[string](/g' *_test.go)
# No need to cast interfaces
( cd generics_tree && $(SED) -i -E -e 's/\.\(string\)//g' *_test.go)
Copy link
Contributor

Choose a reason for hiding this comment

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

@vincentbernat Whats that part of makefile responsible for?
You have used is as part of generating generics_tree and it still can be used somehow?

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 am unsure to understand your question. For generics_tree, the codegen-% rule is used as a first step, but further adaptations are needed. This code does that. The comments should show the different steps, specific to generics.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.. now I understood.. thanks..

@ingwarsw
Copy link
Contributor

@vincentbernat Thanks for contribution.

Please give us some time to dig thru generics code and do internal testing.

@vincentbernat
Copy link
Contributor Author

I have pushed another commit to update the linter action to support Go 1.18. I think that's why it is failing. Also, before merging this PR, I suppose the README would need to be updated as well. Also, I wonder if there is any difference between code using generics and the specialized code. If not, an option would be to simply wait for Go 1.19 to be released, then Go 1.17 will be out of support and the code using generics could become the main one, without needing to generate stuff. Or it could be done in a v2 branch.

@vincentbernat vincentbernat force-pushed the feature/generics branch 4 times, most recently from 85df33d to 0e85eea Compare August 8, 2022 16:59
The code mutation is a bit less pretty but it still works well. Tests
are kept in this case as they are easy to mutate to check that
everything is working.

However, this requires bumping the minimal Go version to 1.18. For
clarity, generated code will be in a separate commit.
@vincentbernat
Copy link
Contributor Author

I have updated the PR to make it work with PR #22.

Copy link
Contributor

@i3149 i3149 left a comment

Choose a reason for hiding this comment

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

I think this looks good to me. At least it will not break anything.

@vincentbernat
Copy link
Contributor Author

Note that it will prevent people using older versions of Go, even when they don't use generics.

@ingwarsw
Copy link
Contributor

ingwarsw commented Aug 8, 2022

I just released v1.1.0 that will be working with go 1.13+ and will release v1.2.0 that will be compatible only with 1.18+
That whould protect people, and give place to upgrading tom latest compatible one..

@ingwarsw ingwarsw merged commit ae6f737 into kentik:main Aug 10, 2022
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.

None yet

4 participants