Conversation
|
To merge this, we need to carefully review the changes. I'd like sinatra helpers to review these changes. |
|
@304 - Stellar work. Thank you so much for this! So many improvements to readability. I have read the PR commit-by-commit, and its shows that you put in some thought into which change should come before the other. It was easy to follow along – each step was separate and followed from the earlier step. A story, if you will. Thanks for that, as well. Solve the conflict, and then we can merge this, I think. As for future work on RuboCop: RuboCop has a concept of a TODO file - a list of remaining issues to be skipped. Having such a file would allow us to keep enabled the rules we want to follow for new code, while at the same time allowing the old code not to trouble that effort. If some of the rules which didn't pass right now are desirable to pass in the future, we could use |
|
@olleolleolle Thank you for the review! 🎉 I was not aware that Rubocop has a concept of a TODO file. It seems to be a really nice way how to proceed with future fixes. Thank you for sharing that! |
|
@304 👋 Hi!!!! Would you mind rebasing again? The note about required Ruby version 2.3+ is also relevant. |
|
Hello folks, could we push this? 🙏 If needed, I could continue with it |
|
I'm thinking this can be merged into v2.1.1. |
It breaks layout of the code, it is better to fix it manually
It affects proc definitions for sinatra DSL
Causes test failures
It would be easier to re-enable them in separate PRs
jkowens
force-pushed
the
feature_add_rubocop
branch
2 times, most recently
from
ac5b509 to
a07289e
Compare
jkowens
force-pushed
the
feature_add_rubocop
branch
from
a07289e to
faed6e4
Compare
| 3.times { get(ping_path) } | ||
| true | ||
| rescue Errno::ECONNREFUSED, Errno::ECONNRESET, EOFError, SystemCallError, OpenURI::HTTPError, Timeout::Error | ||
| rescue EOFError, SystemCallError, OpenURI::HTTPError, Timeout::Error |
There was a problem hiding this comment.
Rubocop was reporting a ShadowedException. It seems SystemCallError would also rescue Errno::ECONNREFUSED, Errno::ECONNRESET errors.
olleolleolle
left a comment
olleolleolle
left a comment
There was a problem hiding this comment.
Thanks, everyone, ❤️ ! This was a barn-raising, many hands needed.
|
This seems to have resulted in an accidental breaking change for direct users of |
It is already set to true for sinatra-contrib and rack-protection, happened in sinatra#1537
It is already set to true for sinatra-contrib and rack-protection, happened in sinatra#1537
7 participants
As suggested in #1531
I would like to introduce rubocop to the project that should help to maintain a consistent code style.
The set of changes is quite big and it will take several PRs to finalize it.
The following PR:
--safe-auto-correctoption)It enables Travis CI checks for the code style violations and that should already bring some value.
Next steps would be to re-enable cops from
.rubocop.ymlfile until the check will pass without any warnings.