-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
LSP: code action to remove redundant tuple in case subject #3057
LSP: code action to remove redundant tuple in case subject #3057
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!!! I'm impressed with how cleanly you've done this.
I've left some comments inline, thank you.
typ: &'ast Arc<Type>, | ||
subjects: &'ast [ast::TypedExpr], | ||
clauses: &'ast [ast::TypedClause], | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is super cool! I love the approach you've taken here, and I think we could use it for lots of other things too.
Could we get more comments that detail what this class does and the approach it uses to doing it. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check, are you referring to the usage of the Visit
trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything! Ideally all logic should have comment explaining the intent in plain English as it's not clear if you either don't have context or are a newcomer to the codebase. Being approachable is super important for Gleam's long-term maintainability as we don't have a big corporate sponsor.
c1823ab
to
0be238d
Compare
0be238d
to
65ab5b9
Compare
Bumping in case you missed this @lpil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely! Really nice! I've left a few small notes inline, once done and the code has been commented I think we're good to go!
Thanks again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!!!
Closes #2982
Supports batch removal.
Also adds a basic visitor pattern to walk the typed AST.