-
Notifications
You must be signed in to change notification settings - Fork 9
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
Merge toolkit-cleanup into develop #218
Conversation
Instead of guessing a possibly wrong default value
[Toolkit Cleanup] Yard doc: step 1 – config cleanup
[Toolkit Cleanup] Yard doc: step 2 – rename module Helpers -> Helper
# Conflicts: # Gemfile # Gemfile.lock
Change the setting of the rule from the default (no_comma) to consistent_comma, which better matches our style
[rubocop] Step 5
Merge develop into cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only skimmed the diff, since all the work that made into toolkit-cleanup
already went through code review.
Approving to avoid congestions, even though this is pending #217, which I just approved with a question.
@@ -0,0 +1,2 @@ | |||
--color |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes for 🎨 !
I think I came across a study (or maybe just an anecdote 🤔 ) on the value of having red for failed and green for passing tests from a psychological point of view. Something like green is calmer so you'll want to get to green fast. Maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue with this, especially with a distributed company like us, is that colors might have different meanings depending on cultures; especially, in China, red is positive and green is negative (*), so the opposite meaning of what western culture is used to. I wonder if rspec and similar tools take the locale into account and switch those accordingly, but I doubt it. AFAIK we don't have anyone working on this codebase living in China, but still a good anecdote to know about.
(*) that's why, for example, in Apple's Stocks app, if you have your device's locale and region set to China, the colors used for positive vs negative fluctuations are switched compared to a device set in any other region.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything here seems to come from reviewed PRs, so I'm ok with
Thanks for all this amazing clean-up @AliSoftware 🙇
Co-authored-by: Gio Lodi <[email protected]>
[Cleanup/rubocop] Step 6
Codecov Report
@@ Coverage Diff @@
## develop #218 +/- ##
===========================================
+ Coverage 45.26% 46.73% +1.47%
===========================================
Files 96 96
Lines 4072 3931 -141
===========================================
- Hits 1843 1837 -6
+ Misses 2229 2094 -135
Continue to review full report at Codecov.
|
Once #217 is merged into
toolkit-cleanup
, I think it's time to merge all the intermediate work back todevelop
(and start using it next time we do a release).Note: I'll still continue to do other PRs for that project but probably directly on develop by now, and better not keep the long-lived branch too long.