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

Detect offenses within class_eval/exec block #55

Merged

Conversation

viralpraxis
Copy link
Collaborator

@viralpraxis viralpraxis commented Sep 26, 2024

Resolves #2

BTW @mikegee maybe it's time for a release? There are a lot of unpublished changes atm 🙂

@viralpraxis viralpraxis self-assigned this Sep 26, 2024
@viralpraxis viralpraxis marked this pull request as ready for review September 27, 2024 21:24
@mikegee
Copy link
Collaborator

mikegee commented Sep 27, 2024

maybe it's time for a release? There are a lot of unpublished changes atm 🙂

I agree. Thank you for that. :)

I added you on rubygems.org. Feel free to bump the version and publish a release. 👍

@viralpraxis
Copy link
Collaborator Author

maybe it's time for a release? There are a lot of unpublished changes atm 🙂

I agree. Thank you for that. :)

I added you on rubygems.org. Feel free to bump the version and publish a release. 👍

Thanks! I'll do it a little bit later, gonna take a look at the changes to make sure they're all fine first

@viralpraxis
Copy link
Collaborator Author

Regarding this PR, it might be that we need to change the offense message to avoid confusion. This cop now reports class instance variables which are not in a method, so "Avoid instance variables in class methods." is a little bit misleading.

@mikegee
Copy link
Collaborator

mikegee commented Sep 27, 2024

Regarding this PR, it might be that we need to change the offense message to avoid confusion. This cop now reports class instance variables which are not in a method, so "Avoid instance variables in class methods." is a little bit misleading.

Makes sense 👍

You could extract a new cop or make the message longer to explain things. 🤷

@viralpraxis
Copy link
Collaborator Author

viralpraxis commented Sep 28, 2024

Regarding this PR, it might be that we need to change the offense message to avoid confusion. This cop now reports class instance variables which are not in a method, so "Avoid instance variables in class methods." is a little bit misleading.

Makes sense 👍

You could extract a new cop or make the message longer to explain things. 🤷

I think the message "Avoid class instance variables" should be good (that's basically what we're trying to say: any class instance variable is potentially unsafe, we mostly track it inside class methods, but that's just an implementation detail/restriction). But it implies we should change cop's name as well (InstanceVariableInClassMethod -> ClassInstanceVariable). We can do it with rubocop's built-in config obsoletion mechanism. @mikegee What do you think about this?

@mikegee
Copy link
Collaborator

mikegee commented Sep 28, 2024

But it implies we should change cop's name as well (InstanceVariableInClassMethod -> ClassInstanceVariable). We can do it with rubocop's built-in config obsoletion mechanism. @mikegee What do you think about this?

That sounds right to me 👍

@viralpraxis
Copy link
Collaborator Author

But it implies we should change cop's name as well (InstanceVariableInClassMethod -> ClassInstanceVariable). We can do it with rubocop's built-in config obsoletion mechanism. @mikegee What do you think about this?

That sounds right to me 👍

Fine 👍🏻, let me do it in separate PR to keep this one simple, is it OK to merge as is?

@viralpraxis viralpraxis merged commit 1cb008a into rubocop:master Sep 29, 2024
18 checks passed
@viralpraxis viralpraxis deleted the detect-offenses-within-class-eval-block branch September 29, 2024 11:09
@viralpraxis
Copy link
Collaborator Author

@mikegee I'l cut 0.6.0 today, everything looks fine

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.

Detect class_eval as class context
2 participants