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

FIX Add missing did you mean gem #9

Merged
merged 4 commits into from Sep 17, 2020

Conversation

simonwgill
Copy link
Contributor

In adding version 0.3.0 to the devops repositories, I noticed that there was an error in the VariableForce cop.

This turns out to be related to an issue with rubocop not being able to find the DidYouMean module. It has been a default module for some time, but for some reason has been removed and is now required in the Gemfile.

Copy link
Contributor

@mrdaniellewis mrdaniellewis left a comment

Choose a reason for hiding this comment

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

Looks good

I'm assuming this works. I'm reading through the issue and I couldn't see a conclusion?

@simonwgill
Copy link
Contributor Author

Looks good

I'm assuming this works. I'm reading through the issue and I couldn't see a conclusion?

Yeah - there's not much of a conclusion in that issue. They dropped the functionality if they can't find the gem, which at least allows rubocop to run, but that stops the VariableForce feature working.

Based on the issue ruby/did_you_mean#117 linked from the rubocop issue, it looks like if there are problems loading did_you_mean through bundle, it should always explode. The difference between not having and adding the gem to a bundle is getting the "error in VariableCop" problem and not getting it.

@luke-hill
Copy link
Contributor

Yeh reading through this issue they can't hypothesise the issue. But given it relates to load paths e.t.c. this being in an issue in thin dockerfiles seems likely.

Indirectly, this problem is also mitigated in version 0.87+ so let's bump the version to 0.87.0+ that way we should avoid it even moreso (They add a return statement before the crash).

@simonwgill
Copy link
Contributor Author

If 0.87.0+ does the right thing with the VariableForce errors, I'm happy to take that in instead of adding the gem. Should we just go to the latest version now though?

I wasn't seeing a crash of the whole process, so I'm not sure what you mean by the additional return statement helping.

@luke-hill
Copy link
Contributor

I honestly don't know. I just had a look at the same thing you linked to. I also looked at The owner's fix that went in (Where he "protects" the call to use the new code with a [] return statement.

The issue is someone altered some logic to use an old piece of DidYouMean functionality, but upon fixing it directly, it's not loading the gem in directly, as rubocop is a CLI not a ruby program per se. I don't really know what we will see without giving it some runs. So maybe update to latest, see if it's good, if not directly referencing the gem is fine by me.

@mrdaniellewis probably has a bit more insight to me. I've not seen any issues on any of the OSS repos I use (But they all use reasonably up to date versions of rubocop). Also we only use ruby 2.6+ in most of our projects, the issues were occuring with ruby 2.3/2.4 mainly I believe.

Fix that would avoid needing to load did_you_mean is apparently in 0.87 so trying that.
Tested this within devops-tools, going to 0.87 does indeed fix the error reporting issue.
@simonwgill
Copy link
Contributor Author

@luke-hill Looks like it's fine just using an upgrade to rubocop.

@simonwgill simonwgill merged commit 2a19a9d into master Sep 17, 2020
@simonwgill simonwgill deleted the fixAdd-missing-did_you_mean-gem branch September 17, 2020 11:23
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.

None yet

3 participants