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

Typed annotations #37

Merged
merged 12 commits into from
Nov 9, 2019
Merged

Typed annotations #37

merged 12 commits into from
Nov 9, 2019

Conversation

milesgranger
Copy link
Owner

Will close #20

@milesgranger milesgranger changed the title [WIP] Typed annotations Typed annotations Nov 3, 2019
@milesgranger milesgranger marked this pull request as ready for review November 3, 2019 05:33
@milesgranger milesgranger requested a review from DarkDrek November 3, 2019 11:52
@DarkDrek
Copy link
Collaborator

DarkDrek commented Nov 3, 2019

I don't think we should mix documentation and annotations.
And naming wise it shouldn't be ModuleAttr and Atrr but Scope-Attribute and Item-Attribute (or something like that).
Because #![warn(...)] can also be the fist thing inside a function and then it applies to the whole function scope.

@milesgranger
Copy link
Owner Author

Ah right, of course. Thank you. 👍

src/gen/annotation.rs Outdated Show resolved Hide resolved
src/gen/annotation.rs Outdated Show resolved Hide resolved
src/gen/module.rs Outdated Show resolved Hide resolved
tests/utilities/mod.rs Outdated Show resolved Hide resolved
@DarkDrek
Copy link
Collaborator

DarkDrek commented Nov 4, 2019

Can you implement the Verify trait for Annotation?

@milesgranger
Copy link
Owner Author

Not sure if it's possible? Doesn't appear Parse is implemented for Attribute https://docs.rs/syn/1.0.7/syn/struct.Attribute.html

So I get this:

  --> tests/utilities/mod.rs:19:6
   |
19 | impl Verify for Annotation {
   |      ^^^^^^ the trait `syn::parse::Parse` is not implemented for `syn::attr::Attribute`

@DarkDrek
Copy link
Collaborator

DarkDrek commented Nov 4, 2019

Can you apply this https://docs.rs/syn/1.0.5/syn/struct.Attribute.html#parsing-from-tokens-to-attribute ? But for this would have to provide a new trait or a supertrait of Verify so it may be too much work

@milesgranger
Copy link
Owner Author

Awesome, I can look at it tomorrow probably. Also, I would not be offended if you feel like hijacking this PR if you want. 👍

@DarkDrek
Copy link
Collaborator

DarkDrek commented Nov 4, 2019

Ok if I find time I can try 👍. I hope you also didn't fell offended by my remarks 👋

@milesgranger
Copy link
Owner Author

haha, never! More comments the better, my Rust skills are a work in progress anyway so I appreciate it!

@milesgranger
Copy link
Owner Author

milesgranger commented Nov 7, 2019

If you're good with it, we can add a separate issue for implementing Verify/Parse for Annotation after this is in. Otherwise it probably won't be until after next week I can finish this. But again, you are welcome to take it over, my feelings won't be hurt. 😄

@milesgranger
Copy link
Owner Author

..in the larger picture, should this rather be called Attribute instead of Annotation?

@milesgranger milesgranger requested a review from DarkDrek November 9, 2019 06:25
@milesgranger milesgranger merged commit d7eb623 into master Nov 9, 2019
@milesgranger milesgranger deleted the typed-annotations branch November 9, 2019 09:24
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.

Add first class support for attribute syntax?
2 participants