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

Auto-linting bug on $? and $CHILD_STATUS #4097

Closed
helloworlddan opened this issue Mar 7, 2017 · 9 comments
Closed

Auto-linting bug on $? and $CHILD_STATUS #4097

helloworlddan opened this issue Mar 7, 2017 · 9 comments
Labels
enhancement good first issue Easy task, suitable for newcomers to the project help wanted

Comments

@helloworlddan
Copy link

The autolinter currently changes $? to $CHILD_STATUS, which breaks on ruby-2.4.0. Maybe $CHILD_STATUS has been removed and/or is deprecated.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 7, 2017

I'm guessing you failed to add require 'English' to your project.

@bbatsov bbatsov closed this as completed Mar 12, 2017
@VivaLosDoyers
Copy link

I would argue that this shouldn't be an auto-corrected change.

This project should strive to cause zero breakage when auto-correcting.

@OddballGreg
Copy link

Just experienced this same issue in an automation script which sent out incorrect notifications as a result of this auto-corrected change.

As a longtime user of rubocop I've come to feel very safe in using the -a flag to resolve issues without breaking the code; so it came as very large surprise that the automatic change actively broke my script rather than just telling me what I'd need to fix the issue like other cops that have the potential of breaking functionality.

@marcandre marcandre reopened this Jun 15, 2020
@marcandre
Copy link
Contributor

marcandre commented Jun 15, 2020

Could we mark Style/GlobalVars as unsafe, or ideally add a require 'english' near the top of the file if it isn't there (or present in the Gemfile)? We should err on the side of caution.

I also opened #8156

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 15, 2020

I think we should aim for require 'english'. If I recall correctly we didn't do in the past, as typically projects tend to just require this once somewhere high up the load sequence, so it's available everywhere. Definitely the cop is unsafe for the vars in English in its current form.

@isaiahfrantz
Copy link

This is an incredibly annoying issue.
I love the auto refactoring that I get for free from rubocop but to modify code to depend on a gem and then not add the include is insane.
We are in the process of fixing up a bunch of puppet modules with the "pdk validate -a" command to auto correct a lot of simple formatting issues. It also refactors a bunch of ruby code in puppet facts and functions in real nice ways. We are left having to do extra manual work after we hit a puppet module that uses $? to check shell out exit codes or our code is broken :(
Seems like the proper thing to do is either auto add the include or dont do the refactor.

@marcandre
Copy link
Contributor

PR welcome to add the require. It's require 'English' with a capital 'E' btw.

@marcandre marcandre added enhancement good first issue Easy task, suitable for newcomers to the project help wanted labels Jul 16, 2020
@marcandre
Copy link
Contributor

Note: cop has been marked as unsafe, so current RuboCop will not autocorrect it with -a, only with -A

@biinari
Copy link
Contributor

biinari commented Jun 21, 2021

I have a branch in the works to add the require 'English' on corrections for these global variables, just going to tidy it up before submitting a PR.

biinari added a commit to Fatsoma/rubocop that referenced this issue Jul 23, 2021
When replacing short global variables with descriptive aliases from English
stdlib module, ensure English module is required.

Adds a new configuration `RequireEnglish` (default `true`) for
`Style/SpecialGlobalVars` cop. When enabled, this will add a
`require 'English'` statement if not already present at the top of the
file.
@bbatsov bbatsov closed this as completed in c3e717f Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue Easy task, suitable for newcomers to the project help wanted
Projects
None yet
Development

No branches or pull requests

7 participants