-
Notifications
You must be signed in to change notification settings - Fork 36
feat: add type parameters #34
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
feat: add type parameters #34
Conversation
flupke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with just a few naming nits. Thanks for showing me how it's done :)
(note: I'm not a maintainer, just commenting)
lib/typed_struct.ex
Outdated
| # A arg named :int | ||
| arg :int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # A arg named :int | |
| arg :int | |
| # A type parameter named int | |
| arg :int |
README.md
Outdated
| use TypedStruct | ||
|
|
||
| typedstruct do | ||
| arg :state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would call it parameter:
- to match the style of
fieldandplugin, which are not abbreviated - to match the documentation, which talks about "parameterized types"
lib/typed_struct.ex
Outdated
| end | ||
|
|
||
| def __arg__(name, _env) do | ||
| raise ArgumentError, "a arg name must be an atom, got #{inspect(name)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| raise ArgumentError, "a arg name must be an atom, got #{inspect(name)}" | |
| raise ArgumentError, "a parameter name must be an atom, got #{inspect(name)}" |
lib/typed_struct.ex
Outdated
| end | ||
|
|
||
| @doc """ | ||
| Defines a argument for a typed struct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Defines a argument for a typed struct. | |
| Defines a type parameter for a typed struct. |
|
Hello, thank you for your PR! @flupke, I’ve also read your comments on the other PR, sorry that both of you have worked on the same subject. I’m still quite loaded on non-OSS personal duties this week, and not available this week-end, but I will review it next week, along with other pending PRs. As stated in the CONTRIBUTING.md, work should be based and merged back to develop. Could you then just rebase your PR on top of Note that I have disabled Elixir 1.14 from the CI as of now, as there is a bug in Elixir 1.14.0 breaking |
2a33f4f to
b31b509
Compare
|
@flupke Thanks for your suggestions that makes sense. @ejpcmac Thanks for your reply, and I’ve updated the PR according to the CONTRIBUTING. |
|
This is an awesome package that saves me a ton of boilerplate. Thanks @ejpcmac! Unfortunately, I really need parameterized types. Is this still being considered, or am I better off forking/creating a plugin? Thanks! |
|
I am extremely grateful to @ejpcmac for providing this package, which has performed excellently in my projects. However, it lacked some features I needed, and it appeared that the repo was not being maintained actively. I rewrite this package with some new functionalities, including those introduced in this PR. For those interested in these enhancements, you are welcome to visit https://github.com/elixir-typed-structor/typed_structor or https://hexdocs.pm/typed_structor/readme.html. Once again, many thanks to @ejpcmac . |
|
Thank you @fahchen for your work and kind words. I’ve actually burned out of OSS maintenance three years ago around a bad pull request experience. I was under pressure at my job at the same moment, also doing a lot of difficult code reviews. I could not handle both. Since then it’s been hard to break the silence, and I’ve mostly avoided to work on I’m sorry you needed to rewrite it, though looking at the code of your fork there seems to be really neat things that could benefit I’m now getting back to maintenance, starting by merging open pull requests and tackling some easy features. I plan to do a new release in the weeks to come. I’ll rebase your PR and handle any change myself. |
b31b509 to
e9c7edc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #34 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 41 45 +4
=========================================
+ Hits 41 45 +4
🚀 New features to boost your workflow:
|
e9c7edc to
2a07149
Compare
|
I have done two pushes:
In push (2) I have updated the test to include two type parameters, and fixed a bug where the type parameters where passed in reverse. I’ll push type parameter documentation support with an additional |
2a07149 to
0f6532f
Compare
Co-authored-by: Phil Chen <06fahchen@gmail.com> Co-authored-by: Jean-Philippe Cugnet <jean-philippe@cugnet.eu>
0f6532f to
8d72169
Compare
a096de5 to
056086d
Compare
related pr #28