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

Add annotations to libraries #335

Merged
merged 1 commit into from Jul 20, 2021

Conversation

donny-dont
Copy link
Contributor

Adds the ability to annotate a Library generated by code_builder. This is to support scenarios like JavaScript interop using package:js where the library itself needs to be annotated with @JS().

@google-cla google-cla bot added the cla: yes label Jun 23, 2021
@natebosch
Copy link
Member

I'm a little hesitant on this one.

Some of our tools allow annotating the first import and treats it as if it was an annotation on the library, but that isn't really the way the grammar works as I understand it. If any tools aren't taking the extra step of reading annotations on the first import this feature will appear misleading.

The slightly more correct way to annotate a library is to have a library statement and a library name. We don't support either in code_builder.

@jakemac53 - WDYT?

If we had dart-lang/language#1073 this would be easy - we could always add a library;.

We could potentially add this feature for now with a comment calling out that support may be inconsistent, and then update it if we get that language feature.

@donny-dont
Copy link
Contributor Author

Would you be against Library optionally having a name then?

In this case if you add an annotation but not a name then it could throw and then later if the language allows a nameless library declaration and you're targeting a version of Dart with that it removes the restriction.

@jakemac53
Copy link
Contributor

Ya I think we should require a named library from the user if we want to allow annotating it.

@donny-dont
Copy link
Contributor Author

@jakemac53 @natebosch should I also mixin Library with HasDartDocs and if a name isn't present throw as well while I'm here?

@donny-dont donny-dont force-pushed the library-annotations branch 2 times, most recently from c5bba7e to e38b032 Compare June 23, 2021 23:53
@donny-dont
Copy link
Contributor Author

Ok I just added the annotations part so I think this is good to review.

@donny-dont
Copy link
Contributor Author

@natebosch @jakemac53 the library name is now generated. Happy to add documentation as well if that's something that you all want. Wasn't sure since #311 implied documentation might get a revamp.

@jakemac53
Copy link
Contributor

This generally LGTM but I will let Nate have the final say as he is more involved in this package

@donny-dont
Copy link
Contributor Author

@natebosch any thoughts on this?

@natebosch natebosch merged commit 7843ff3 into dart-lang:master Jul 20, 2021
@natebosch
Copy link
Member

@donny-dont - sorry for the delay. This is published now.

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

3 participants