Adopt standard for linting #90
Conversation
There was a problem hiding this comment.
I love the idea of adopting a linter to help ease the developer experience and avoid code style bikeshedding. And it's always an ambitious project to make happen in an existing repo. And honestly I don't have strong feelings about the specific linting rules at this point, so picking an opinionated framework like standard ruby makes sense to me.
At the moment these are the blockers in this PR:
- Must: Update the README so that the "Lend a Hand" section includes information about using the linter.
- Must: Fix merge conflicts with the master branch (Not sure if it's easier to re-apply linter fixes vs resolving the many merge conflicts.
Other would-like changes:
- Integrate the linter with
rake guardsince that's already an option for unit tests - After I fix the CI pipeline, we/I should integrate the linter as a step in CI.
Hope this helps! Let me know what you're thinking.
|
|
||
| gemspec | ||
|
|
||
| gem 'coveralls', require: false | ||
| gem "coveralls", require: false | ||
| gem "standard" |
There was a problem hiding this comment.
Perhaps doesn't matter, but it surprised me that the standard gem wasn't within the :test group like the Standard README shows.
There was a problem hiding this comment.
Honest question: how do you use bundle groups for gem development?
I tend to think of gems as having only direct dependencies and development dependencies. Any gem in the Gemfile is effectively a "non-direct" dependency. I could imagine a use case, maybe CI?, where one to filter out a certain subset of dependencies but I couldn't find a place where this gem might be doing that now.
For app development, the use cases are more clear since frameworks (in other words, Rails) bake in the notion of separate environments.
There was a problem hiding this comment.
Honest answer, I have no clue! My Ruby background is primarily from working on Rails SaaS apps so I'm not sure about library development requirements/best practices. Also my day job has taken me away from Ruby a bit so I'm feeling rusty.
Thinking about this a bit more deeply, I suppose it's the ruby-oembed.gemspec file that specifies the peer dependencies that apps consuming this gem will be installing.
Meanwhile the Gemfile I suppose is only used for local development.
So if anything I'd lean towards removing the groups from the Gemfile (since they don't currently appear to be in use) in an attempt to avoid future confusion.
Currently the lint check is appended to the default rake task. That means it should already be integrated into CI since you're using the default task in the new GH action. |
I chose to disable the linting rule for this case because it is test assertion on a generated regexp whereas I believe the spirit of the rule is intended more for hand-written regexps.
Seems to read better when disabling this rule inline
From the Rubocop docs: In Ruby 2.3 or later, use unary plus operator to unfreeze a string literal instead of `String#dup` and `String.new`. Unary plus operator is faster than `String#dup`. Note: `String.new` (without operator) is not exactly the same as `+''`. These differ in encoding. `String.new.encoding` is always `ASCII-8BIT`. However, `(+'').encoding` is the same as script encoding(e.g. `UTF-8`). So, if you expect `ASCII-8BIT` encoding, disable this cop.
|
Oh my! The output from the standard.rb linting is very subtle! For the test runs where I first added GitHub Actions I noticed that the test passed despite the coverall data submission failing so I just kinda left it as-is & decided to try & fix that later. However, because the standard.rb output is currently only shown in grey text it's really blends in with the rest of the text, and is currently entirely over-powered by the coveralls errors.
Of course I need to fix the coveralls issue separately from this PR (maybe even disabling it for now), but/and it would still be awesome if the linting output was more prominent somehow. |
This PR adds standard for Ruby linting.
I can see that a branch started for rubocop several years old but that also included quite a bit of refactoring. I took a conservative approach to avoid rewriting library code if possible. Standard is a wrapper around Rubocop so the lint changes are "in the spirit" of the original PR.
For review, it may make sense to review by commit; commit 24bd6b8 is one where I used
standardrb --fix, the other lint errors I fixed by hand.