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

Issue #1081: non necessary raw strings #1178

Merged
merged 28 commits into from Oct 20, 2020

Conversation

Mema5
Copy link
Contributor

@Mema5 Mema5 commented Feb 23, 2020

First PR for our group

@fwald and I solved this issue today. We added the new rule along with tests and documentation. We also refactored old tests and enforced the new rule in the project.
Thank you for your answers!

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

🙏 Please, if you or your company is finding wemake-python-styleguide valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/wemake-python-styleguide. As a thank you, your profile/company logo will be added to our main README which receives hundreds of unique visitors per day.

@Mema5 Mema5 requested a review from sobolevn February 23, 2020 18:16
@sobolevn
Copy link
Member

Wow! @Mema5, @Gwin73 thanks! I will review this shortly.

Offtop: can you please share your story behind this feature? Do you practice pair programming? Or is it a part of some kind of practice? And the most important question for me as a maintainer: why did you choose this project to contribute to?

You rock! 👾 🚀

@Mema5
Copy link
Contributor Author

Mema5 commented Feb 23, 2020

Thank you, it is rewarding to receive such a positive feedback :)

Actually we are a group of 5 students (@fward @Gwin73 @Fils2 @ruwaid123 @Mema5) and we contribute as part of a course assignment. So yes, it is mostly to learn and gain experience on software engineering practices!
As for the project selection, we spent some time on that and we chose WPS for the following reasons. First, the project was active (thanks to you), the issues seemed interesting and reachable for us. Secondly, the building and testing were relatively easy from the documentation, thanks to poetry. Finally, the members of our team were pleased with the purpose of the project and keen on working in Python :)

The project will only last one week, but we are glad if it is useful!

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 for your work! 👍
There are several minor things to polish and I will schedule this PR for 0.15 release.

CHANGELOG.md Outdated Show resolved Hide resolved
wemake_python_styleguide/violations/consistency.py Outdated Show resolved Hide resolved
wemake_python_styleguide/violations/consistency.py Outdated Show resolved Hide resolved
wemake_python_styleguide/violations/consistency.py Outdated Show resolved Hide resolved
modifiers, string_def = split_prefixes(token)

if 'r' in modifiers.lower():
if '\\' not in string_def.encode('unicode-escape').decode():
Copy link
Member

Choose a reason for hiding this comment

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

r'\' is better here.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please also explain encode('unicode-escape').decode() part? Why is it required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, r'\' is not correct in Python I think.

Python 3.6.9
>>> a=r'\'
  File "<stdin>", line 1
    a=r'\'
         ^
SyntaxError: EOL while scanning string literal
>>> 

The encode('unicode-escape').decode() is a workaround to convert into a raw string. (see here). If you know something more beautiful let me know!

Copy link
Member

@sobolevn sobolevn Feb 23, 2020

Choose a reason for hiding this comment

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

Ok, about r'\' part 👍

Can you please test your code without encode('unicode-escape').decode()? Will it still work?

Copy link
Member

Choose a reason for hiding this comment

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

@Mema5 have you tried running tests without encode('unicode-escape').decode()? Did it work?

wemake_python_styleguide/visitors/tokenize/primitives.py Outdated Show resolved Hide resolved
Mema5 added a commit to adrwes/wemake-python-styleguide that referenced this pull request Feb 23, 2020
Mema5 added a commit to adrwes/wemake-python-styleguide that referenced this pull request Feb 24, 2020
* CHANGELOG.md: new header for release 0.15.0
* added contribution
* Related to PR wemake-services#1178
Mema5 added a commit to adrwes/wemake-python-styleguide that referenced this pull request Feb 24, 2020
tests/fixtures/noqa/noqa.py Outdated Show resolved Hide resolved
modifiers, string_def = split_prefixes(token)

if 'r' in modifiers.lower():
if '\\' not in string_def.encode('unicode-escape').decode():
Copy link
Member

Choose a reason for hiding this comment

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

@Mema5 have you tried running tests without encode('unicode-escape').decode()? Did it work?

Mema5 added a commit to adrwes/wemake-python-styleguide that referenced this pull request Feb 24, 2020
Mema5 added a commit to adrwes/wemake-python-styleguide that referenced this pull request Feb 24, 2020
Mema5 added a commit to adrwes/wemake-python-styleguide that referenced this pull request Feb 24, 2020
Mael MADON and others added 19 commits February 25, 2020 00:08
…e '\' character. If there is no '\' in the string a raw string should not be used.

Closes issue #8
Relates to issue#1081 on the original repository
…edViolation (WPS357). Follows the general structure for documentation.

Relates to original issue wemake-services#1081. Closes #11
…some cases to make the test pass. Related to issue #9
* CHANGELOG.md: new header for release 0.15.0
* added contribution
* Related to PR wemake-services#1178
@sobolevn
Copy link
Member

@Mema5 can you please rebase your branch?

@Mema5
Copy link
Contributor Author

Mema5 commented Feb 25, 2020

@sobolevn here it is!

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.

Almost done! The last round of review.

There are just 3 minor notes and we are ready to go.
Thank you!

wemake_python_styleguide/violations/consistency.py Outdated Show resolved Hide resolved
wemake_python_styleguide/violations/consistency.py Outdated Show resolved Hide resolved
wemake_python_styleguide/visitors/tokenize/primitives.py Outdated Show resolved Hide resolved
@coveralls
Copy link

Pull Request Test Coverage Report for Build 2656

  • 10 of 10 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2650: 0.0%
Covered Lines: 5243
Relevant Lines: 5243

💛 - Coveralls

@coveralls
Copy link

coveralls commented Feb 25, 2020

Pull Request Test Coverage Report for Build 2664

  • 10 of 10 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2650: 0.0%
Covered Lines: 5223
Relevant Lines: 5223

💛 - Coveralls

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! Your work will be merged into 0.15

@sobolevn sobolevn added this to the Version 0.15 milestone Feb 25, 2020
@Mema5 Mema5 changed the title Issue #1081 Issue #1081: non necessary raw strings Feb 26, 2020
@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #1178 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1178   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          105       105           
  Lines         5531      5540    +9     
  Branches      1224      1226    +2     
=========================================
+ Hits          5531      5540    +9     
Impacted Files Coverage Δ
wemake_python_styleguide/logic/naming/access.py 100.00% <100.00%> (ø)
wemake_python_styleguide/violations/consistency.py 100.00% <100.00%> (ø)
..._python_styleguide/visitors/tokenize/primitives.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34b9004...f9a38f7. Read the comment docs.

@sobolevn sobolevn merged commit 9dd22ac into wemake-services:master Oct 20, 2020
@sobolevn sobolevn modified the milestones: Version 0.16, Version 0.15 aka New runtime Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raw strings vs regular strings
4 participants