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

Implement named/pos_args annotation methods #7694

Merged

Conversation

@Blacksmoke16
Copy link
Contributor

commented Apr 19, 2019

Resolves #7692.

Adds #named_args and #args methods to Annotation.

Show resolved Hide resolved spec/compiler/semantic/annotation_spec.cr Outdated
Show resolved Hide resolved spec/compiler/semantic/annotation_spec.cr
Show resolved Hide resolved src/compiler/crystal/macros.cr Outdated
@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Good for review again @bcardiff @straight-shoota

@bcardiff
Copy link
Member

left a comment

LGTM but it needs a rebase on master to make the CI happy

@Blacksmoke16 Blacksmoke16 force-pushed the Blacksmoke16:annotation-arg-methods branch from 79d7283 to 7c89b71 Apr 24, 2019

@bew

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Imo args should return something with pos + named args, that why i suggested pos_args in the first place to target only positional args. (the same comment goes for Call)

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@bcardiff @bew Would it be worth to add pos_args back in addition to the other two? Then you could either target all args, only named, or only positional?

If so It would be pretty easy to do and I could take care of that today.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Thinking of this more, the challenge with returning them both would be how to do that, since they are of different "types". Could have #args return like Tuple(Tuple(Positional), NamedTuple(Named)) Or even a named tuple like NamedTuple(positional: Tuple(Positional), named: NamedTuple(Named)).

@Blacksmoke16 Blacksmoke16 force-pushed the Blacksmoke16:annotation-arg-methods branch from 7c89b71 to b49f8df Jul 24, 2019

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@bcardiff @bew @straight-shoota Could you review this again? I added #args method that will return a named tuple representing both positional and named args.

{named: {foo: "bar"}, positional: {1, true, 17}}

So now it's possible to get positional arguments, named arguments, or both off of an annotation.

@RX14

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

I'd prefer not to return a named tuple, no other macro methods behave like this. I'm a bit lost to the usecase of #args...

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

@RX14 I'd also be fine with removing #args and just keeping #pos_args and #named_args.

@RX14

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@Blacksmoke16 that would be great. In fact, I'd just name #pos_args as #args. I'd like others feedback though.

@bcardiff

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Using only args (for positional args) and named_args to match the AST seems the best alternative to me.

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@bcardiff
Copy link
Member

left a comment

Eventually, I would like to see a more structured API for annotations. I don't think this will be a definitive API for this. But is serves for now. So, to be used it with caution :-)

@Blacksmoke16

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Works for me. This is a big improvement over my current hack I'm using :P

@RX14

RX14 approved these changes Jul 29, 2019

@RX14 RX14 added this to the 0.30.0 milestone Jul 29, 2019

@straight-shoota straight-shoota merged commit ae7b538 into crystal-lang:master Jul 29, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@straight-shoota

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Thank you @Blacksmoke16

@Blacksmoke16 Blacksmoke16 deleted the Blacksmoke16:annotation-arg-methods branch Jul 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.