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 rule for missing label on exit/cycle #53

Merged
merged 4 commits into from
Oct 2, 2024
Merged

Conversation

ZedThree
Copy link
Member

@ZedThree ZedThree commented Oct 2, 2024

Mostly following the equivalent rule in stylist except:

  • also capture cycle as well as exit
  • only applies to do loops with a name
  • unfortunately, warns twice for unlabelled exit/cycle` in nested loops

Last point could probably be fixed with some more sophisticated logic: walking the tree, and only descending into non-do nodes. Then I think we'd be able to confidently suggest the correct label for a fix.

Second point could be turned into another rule: exit/cycle in nested loops should be labelled.

Also, I've just put this into the "style" category, but I'm not sure that's completely appropriate. Maybe we need some community input into the categorisation of rules.

@LiamPattinson
Copy link
Collaborator

Second point could be turned into another rule: exit/cycle in nested loops should be labelled.

I think this is a good idea, especially since the PR I raised into this branch will result in no warning if an inner loop containing an exit/cycle is unlabelled. Please could you raise a new issue for it?

Also, I've just put this into the "style" category, but I'm not sure that's completely appropriate. Maybe we need some community input into the categorisation of rules.

I'm also not sure about this, as this feels more like ensuring correctness instead of enforcing purely stylistic rules. However, it doesn't fit any of the other categories, and I can't immediately think of new category to introduce, so I think it would be best to leave it here for now.

Copy link
Collaborator

@LiamPattinson LiamPattinson left a comment

Choose a reason for hiding this comment

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

All looks good, especially after passing the label into the error message!

@LiamPattinson LiamPattinson merged commit 646837c into main Oct 2, 2024
22 checks passed
@ZedThree ZedThree deleted the exit-with-no-label branch November 14, 2024 14:21
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