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

Class annotation support #270

Merged
merged 18 commits into from
Jan 17, 2021
Merged

Conversation

georgejecook
Copy link
Contributor

@georgejecook georgejecook commented Jan 13, 2021

Adds annotation support for classes, attaching any pending annotation…s that appear before a class keyword

@elsassph inspired (i.e goaded) me to add annotation support for maestro, perhaps knowing that this feature was missing and I would therefore have to write it :D Great delegation @elsassph 👏

Hopefully this approach is okay - I'm sure we all concur that annotations are just as usefu on classes as other statements.

@elsassph
Copy link
Contributor

elsassph commented Jan 13, 2021

Good catch - though annotations shouldn't normally be added from inside the statement readers normally. I need to check.

@georgejecook
Copy link
Contributor Author

I also see that we're missing it for class methods (at least that seems to be the case):
image

If so, I can add that.

If there's a more proper way to implement this, let me know - it's late, and I quickly moved over maestro and rooibos, so I wasn't expecting to jump back into bsc right now, so just did what was expedient.

Copy link
Member

@TwitchBronBron TwitchBronBron left a comment

Choose a reason for hiding this comment

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

Looks simple enough to me. I have requested two small changes, but then it's good to go..

@@ -460,6 +463,12 @@ export class Parser {
parentClassName,
this.currentNamespaceName
);

//attach annotations to statements
if (classAnnotations?.length) {
Copy link
Member

Choose a reason for hiding this comment

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

While this technically works due to javascripts truthy/falsey, let's check for length > 0

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 got this from other code in the project.. I didn't like it; but I thought it was idiomatic

Copy link
Contributor

Choose a reason for hiding this comment

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

Test should be

if (classAnnotations.length > 0) {

and we can merge - I'll complete some other things.

src/parser/Parser.spec.ts Outdated Show resolved Hide resolved
@TwitchBronBron TwitchBronBron changed the title Adds annotation support for classes, attaching any pending annotation… Class annotation support Jan 14, 2021
@georgejecook
Copy link
Contributor Author

georgejecook commented Jan 14, 2021

Got a couple of questions about this. @elsassph says this is working out the box as it stands. I didn't get that behaviour with the above unit test so I'm confused.

Secondly I've been told that this should be implemented with refactoring the parser. Don't have time to do that.

Can we just make it work for the time being for classes and class methods and do whatever refactoring later? The feature is half baked as it is imho.

@georgejecook
Copy link
Contributor Author

georgejecook commented Jan 14, 2021

I think I've covered everything here - see tests. called out a line I'd like special revision on (removed the code in question). all tests are passing, so I think we're okay.

src/parser/Parser.ts Outdated Show resolved Hide resolved
@georgejecook
Copy link
Contributor Author

I think this is good to go now. I addressed both of your feedback and merged current master.

@TwitchBronBron TwitchBronBron merged commit f152ca1 into master Jan 17, 2021
@TwitchBronBron TwitchBronBron deleted the fix/add-annotations-to-classes branch January 17, 2021 02:54
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.

3 participants