-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Pass over all Rails 5 warnings #24492
Conversation
Unrelated test failure. |
For deprecation warnings, |
Gah. Dumb of me. Anyway, will get others feedback if we want to just keep this now, else will remove those from this patch. |
I'd keep the periods you added. It feels strange to auto-add missing punctuation rather than properly punctuating the warnings in the first place. Turns out I added this, maybe to avoid updating the many warning messages. I'd prefer to see it go 😁 |
@@ -65,7 +65,6 @@ def deprecated_method_warning(method_name, message = nil) | |||
|
|||
def deprecation_message(callstack, message = nil) | |||
message ||= "You are using deprecated behavior which will be removed from the next major or minor release." | |||
message += '.' unless message =~ /\.$/ |
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.
Would you squash the revert onto the other commit? Then we have a single commit that drops the period-addition behavior and adds missing periods. |
- we are ending sentences properly - fixing of space issues - fixed continuity issues in some sentences. Reverts rails@8fc97d1 . This change reverts making sure we add '.' at end of deprecation sentences. This is to keep sentences within Rails itself consistent and with a '.' at the end.
1616fc4
to
ac02733
Compare
Squashed. |
Yes. Perhaps update the PR title to something like "Clean up warning messages and revert auto-adding periods." |
Pass over all Rails 5 warnings, to make sure:
This is continuation of #24485 and takes care of warnings at all places.