Skip to content
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

Allow dot and comma to be overused string literals #2210

Merged
merged 13 commits into from
Dec 13, 2021
Merged

Allow dot and comma to be overused string literals #2210

merged 13 commits into from
Dec 13, 2021

Conversation

0anton
Copy link
Contributor

@0anton 0anton commented Oct 5, 2021

I have made things!

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

Closes #2209

Remarks:

  • Added quotes to the reported overused string literal. I had bad time looking for the unescaped dot . in logs. Now the log will write it like '.'
  • Replaced wording string constrant overuse with string literal overuse. See for example here for wording usage.
  • Fixed wording in the unit test to better describe what it does.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

@0anton
Copy link
Contributor Author

0anton commented Oct 6, 2021

Thanks for approving, @sobolevn ! I see that the test is breaking. This is possibly related to the change in the format of the error message, I introduced. Do I have to look at it?

@sobolevn
Copy link
Member

sobolevn commented Oct 6, 2021

Yes, seems related. Can you please fix it?

@0anton
Copy link
Contributor Author

0anton commented Oct 6, 2021

I fixed failing tests with this 41d090c. Could you @sobolevn please let the pipeline run again?

@0anton
Copy link
Contributor Author

0anton commented Oct 6, 2021

Ah, my fault that tests fail again: I checked that only test_overused_string.py is successful, not the complete test suit. I'll think of how to improve it and come back.

@sobolevn sobolevn merged commit d27209b into wemake-services:master Dec 13, 2021
@sobolevn
Copy link
Member

Thank you!

@0anton
Copy link
Contributor Author

0anton commented Dec 16, 2021

Thank you for fixing tests and bringing this contribution for acceptance!

@0anton 0anton deleted the allow-dot-comma-constant-overuse branch December 16, 2021 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add exception to WPS226 to allow commas and dots
3 participants