Minor cleanup to regexp for correctness. #458
Conversation
| version = suse.scan(/VERSION = (\d+)\nPATCHLEVEL = (\d+)/).flatten.join('.') | ||
| # https://rubular.com/r/b5PN3hZDxa5amV | ||
| version = suse[/VERSION\s?=\s?"?([\d\.]{2,})"?/, 1] if version == '' | ||
| version = suse[/VERSION *= *("?)([\d\.]{2,})\1$/, 2] if version == '' |
There was a problem hiding this comment.
I find this harder to read ( *= * almost reads as an operator), and you've removed a helpful comment. Please have some mercy on your fellow developers, and keep/update explanatory comments.
There was a problem hiding this comment.
I don't think your intention was to imply malice, but 'please have some mercy' reads kinda harshly.
There was a problem hiding this comment.
First, I agree with @miah here. There’s another PR comment with “be kind and ...” that also reads like you’re implying malicious intent. I don’t think you are, but still... this is a public forum.
Second, The rubular comments seem very inconsistently applied to our projects... tho I can now see that they’re used more in ohai & chef, but it’s still only 18 references across 12 files (out of 3358 files).
Third, I personally don’t think this regexp is that hard to follow as far as our regexps go. I’ll add it back if you want, but I’d rather do a brown-bag (or whatever the remote equivalent would be) on regexps instead. Miah has already said that would help her and I would be happy to do it.
Finally… For fun, I wrote a quick script to find and “score” all our regexps and … well… let’s just say that there are going to be some more issues filed shortly as it is exposing some problems.
zenspider
force-pushed
the
zenspider/minor/regexp-cleanup
branch
from
fa92142
to
b4cc4cc
Compare
|
Code Climate has analyzed commit b4cc4cc and detected 0 issues on this pull request. View more on Code Climate. |
I'm just being a tad anal retentive here. I don't want the quotes to be mis-matched and I don't want the \s to match NLs. Signed-off-by: Ryan Davis <zenspider@chef.io>
zenspider
force-pushed
the
zenspider/minor/regexp-cleanup
branch
from
b4cc4cc
to
0dec81d
Compare
| @@ -239,8 +239,8 @@ def self.load | |||
| unless (suse = unix_file_contents("/etc/SuSE-release")).nil? | |||
| # https://rubular.com/r/UKaYWolCYFMfp1 | |||
| version = suse.scan(/VERSION = (\d+)\nPATCHLEVEL = (\d+)/).flatten.join(".") | |||
| # https://rubular.com/r/b5PN3hZDxa5amV | |||
| version = suse[/VERSION\s?=\s?"?([\d\.]{2,})"?/, 1] if version == "" | |||
| # https://rubular.com/r/xegcPXKEXJrJl5 | |||
I'm just being a tad anal retentive here. I don't want the quotes to
be mis-matched and I don't want the \s to match NLs.
Signed-off-by: Ryan Davis zenspider@chef.io