Skip to content

Fixes NoUnusedVariablesRule false positive #133

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

Conversation

NeedleInAJayStack
Copy link
Member

@NeedleInAJayStack NeedleInAJayStack commented Nov 6, 2023

Fixes #61

@NeedleInAJayStack NeedleInAJayStack changed the title Fixes NoUnusedVariablesRule Fixes NoUnusedVariablesRule false positive Nov 6, 2023
@NeedleInAJayStack NeedleInAJayStack force-pushed the fix/no-unused-variables-rule branch from 93bdc5a to 192b2a9 Compare November 6, 2023 07:30
Copy link
Member

@paulofaria paulofaria left a comment

Choose a reason for hiding this comment

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

Can we add a test with the unhappy path? Also, does the update to the visitor now allows us to do edits? This was something that was quite hard to implement using the TS code as a reference due to some concepts not existing in Swift, making it only being possible with a different Visitor design. Also, if I remember correctly, mutable visitor was also required to unlock many features. One of them would be to export SDLs.

@NeedleInAJayStack NeedleInAJayStack force-pushed the fix/no-unused-variables-rule branch from 192b2a9 to d167dad Compare November 6, 2023 19:48
@NeedleInAJayStack NeedleInAJayStack force-pushed the fix/no-unused-variables-rule branch from d167dad to 2cc0630 Compare November 6, 2023 19:48
@NeedleInAJayStack
Copy link
Member Author

Can we add a test with the unhappy path?

Sure, I added a testVariableUnusedInsideObject test function

Also, does the update to the visitor now allows us to do edits?

Actually, I went down the visitor refactors mostly just to get familiar with the system, but I don't think they're critical to the fix of this rule. I've removed them from this PR and I'll start up a separate one.

@NeedleInAJayStack NeedleInAJayStack force-pushed the fix/no-unused-variables-rule branch from 4b9aaab to 8a1c8f3 Compare November 6, 2023 19:54
Copy link
Member

@paulofaria paulofaria left a comment

Choose a reason for hiding this comment

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

LGTM

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.

False positive with no unused variable rule
2 participants