search

gshutler / useragent

HTTP User Agent parser
MIT License
179 stars 159 forks source link

Rubocop enforcement and simplecov #47

Closed stephengroat closed 4 years ago

stephengroat commented 6 years ago

i know it's huge, if you'd like to minimize the rule impact let me know!

rubocop now runs with rake as the default

most corrections were done with rubocop --auto-correct, some was ignored in .rubocop.yml. if there's a specific style change you'd like to enforce, let me know!

gshutler commented 6 years ago

Thanks for this. I've had a skim and there's a few things that stand out:

  1. The "prefer '' over """ is responsible for 99% of the noise, could we suppress that one (at least temporarily) and then we might see the wood for the trees?
  2. Very wary of changing constants from PascalCase to SNAKE_CASE as people could rely on them as they're part of the public API (sort of)
  3. Similar goes for method names, would either leave as they are or alias-and-warn the old name
  4. I saw a big change to avoid def self.foo in lib/user_agent/operating_systems.rb which I think changes the accessibility of the WINDOWS constant, I'm fine with def self.foo myself
  5. I would be ok with keeping the old => hash style, or at least making that change it's own commit a bit like the string quotation for the sake of clarity in commit history/review
stephengroat commented 6 years ago

Let me see what i can do with .rubocop.yml

stephengroat commented 6 years ago

@gshutler See below

  1. the Style/StringLiterals#double_quotes lessened the noise, but there's still enough difference that it creates some (since it enforces consistency)
  2. added aliases with warnings for any constants that were changed to SCREAMING_SNAKE_CASE and tested them
  3. only found one method name that rubocop wanted changed (has_wmfsdk? to wmfsdk?), added an alias and warned
  4. moved WINDOWS outside of the class context of the module, can access now with UserAgent::OperatingSystems::WINDOWS or UserAgent::OperatingSystems.Windows
  5. Updated with enforcing hash_rockets, didn't help as much as I had hoped
stephengroat commented 6 years ago

think i addressed everything you're looking for (user_agent_spec now has tests showing warn once and never again for a instance, could be for a class with @@ if you want it to be)

stephengroat commented 6 years ago

@gshutler i'm sure you're super busy, i just want to make sure that this pops back up on your radar (if there's anything else you need from me)

stephengroat commented 4 years ago

any interest in this? trying to cleanup my open PRs and wondering if i should just close this. if so, no worries