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

Allow private attribute on aliases #1434

Merged
merged 2 commits into from
Dec 20, 2022
Merged

Conversation

neunenak
Copy link
Contributor

@neunenak neunenak commented Dec 4, 2022

No description provided.

@neunenak
Copy link
Contributor Author

@casey could you take a look at this one when you have a chance?

@casey
Copy link
Owner

casey commented Dec 17, 2022

Yoo, sorry for slacking! This looks mostly good, but I believe the parser will incorrectly parse a recipe named alias with attributes:

[private]
alias:
  echo foo

Just doesn't really have keywords, so alias, let, etc are all allowed to be identifiers. I took a look at the parsing code, but it isn't obvious to me how to fix this easily.

@neunenak
Copy link
Contributor Author

@casey looks like I can use the same strategy the other alias-parsing code was using, namely explicitly looking ahead for a ColonEquals token. This is a bit messy, but not messier than other existing parsing code I think.

@casey
Copy link
Owner

casey commented Dec 19, 2022

I noticed you just pushed some commits, haven't looked at them in detail, but the approach of checking the next few tokens seems reasonable.

One alternative is that we could parse attributes as items, and then let the analyzer figure out which attributes belonged on which items. This would allow attributes to be added to everything, without having to introduce special-purpose code to the parser.

Or we could take the approach used for doc-comments, where the parser basically stores pending doc-comments, and then adds them to recipes when it gets to them.

@casey
Copy link
Owner

casey commented Dec 19, 2022

What do you think? Since you got it working, putting off doing something more general is fine for now.

@neunenak
Copy link
Contributor Author

In the short term, I'd like to get this merged and be able to close out #1421 entirely. Longer-term, I think the current parser is a bit messy in a way that makes making changes to the grammar harder than it needs to be, and it's worth thinking about how to make that better in general rather than just trying to solve individual proposed changes to the grammar as they come up.

One alternative is that we could parse attributes as items, and then let the analyzer figure out which attributes belonged on which items. This would allow attributes to be added to everything, without having to introduce special-purpose code to the parser.

I'm not a huge fan of this idea - this is basically introducing a new variant of Item whose sole purpose is to be merged into the existing nodes after the analyze step. Ideally the parser would be smart enough to figure out that aliases and recipes might or might not have annotations, and add those annotations directly to the AST node objects. It doesn't really make sense for the other sorts of item to have annotations anyway, I don't think.

Or we could take the approach used for doc-comments, where the parser basically stores pending doc-comments, and then adds them to recipes when it gets to them.

This is actually kind of messy in the existing code, in that pop_doc_comment needs to be called in a few different places and the code surrounding it needs to keep track of the mutable eol_since_last_comment flag. Obviously this works, but ideally the parser would have some doc-comment-parsing code that only needs to be called once and could manage its own state internally.

@casey
Copy link
Owner

casey commented Dec 20, 2022

Sounds reasonable to me! Let's get this in.

@casey casey merged commit fbe1c4c into casey:master Dec 20, 2022
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.

2 participants