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

Raw strings vs regular strings #1081

Closed
sobolevn opened this issue Dec 18, 2019 · 7 comments · Fixed by #1178
Closed

Raw strings vs regular strings #1081

sobolevn opened this issue Dec 18, 2019 · 7 comments · Fixed by #1178
Assignees
Labels
help wanted Extra attention is needed level:starter Good for newcomers pr-available rule request Adding a new rule

Comments

@sobolevn
Copy link
Member

One can use both r'abc' and 'abc' to represent a simple string.
But, r strings are only required when dealing with \ in your strings.
So, the rule is: if there's no \ in your string, then do not use r

@sobolevn sobolevn added help wanted Extra attention is needed level:starter Good for newcomers rule request Adding a new rule labels Dec 18, 2019
@sobolevn sobolevn added this to the Version 0.15 milestone Dec 18, 2019
@sobolevn
Copy link
Member Author

This is an inverse rule to WPS342

@fwald
Copy link
Contributor

fwald commented Feb 23, 2020

Hi! I would like to take on this issue if I may?

@sobolevn
Copy link
Member Author

Sure, @fwald! Thanks a lot! Feel free to ask any questions.

@fwald
Copy link
Contributor

fwald commented Feb 23, 2020

When implementing this, the linter complains about this expression in the file
constants.py:

# Used as a special name patterns for unused variables, like _, __:
UNUSED_VARIABLE_REGEX: Final = re.compile(r'^_+$')

Can this be changed to a normal string instead?

fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 23, 2020
…edViolation (WPS357). Follows the general structure for documentation.

Relates to original issue wemake-services#1081. Closes #11
@sobolevn
Copy link
Member Author

@fwald yes please! That's what I love about static analysis the most: finding new issues in code that you have seen so many times!

@Mema5
Copy link
Contributor

Mema5 commented Feb 23, 2020

Hi, I'm working with @fwald on the issue. Enforcing this new rule also breaks tests in the test suite, so I'm working on modifying it.

But I struggle with this error when running pytest tests/test_visitors/test_tokenize/test_primitives/test_string_tokens/test_string_multiline.py:

___________________ test_incorrect_multiline_strings[other_var + {0}-regular_wrapper-r'''\n'''] ____________________

parse_tokens = <function parse_tokens.<locals>.factory at 0x7ffacd7592f0>
assert_errors = <function assert_errors.<locals>.factory at 0x7ffacd759158>
default_options = options(min_name_length=2, max_name_length=45, i_control_code=True, i_dont_control_code=True, max_noqa_comments=10, ne...el=4, max_attributes=6, max_cognitive_score=12, max_cognitive_average=8, max_call_level=3, max_annotation_complexity=3)
primitives_usages = 'other_var + {0}', primitive = "r'''\n'''"
mode = <function regular_wrapper.<locals>.factory at 0x7ffacd5c5598>

    @pytest.mark.parametrize('primitive', [
        '"""abc"""',
        "'''abc'''",
        '""""""',
        "r'''\n'''",
        'b"""some"""',
    ])
    def test_incorrect_multiline_strings(
        parse_tokens,
        assert_errors,
        default_options,
        primitives_usages,
        primitive,
        mode,
    ):
        """Ensures that incorrect multiline strings are forbiden."""
        file_tokens = parse_tokens(mode(primitives_usages.format(primitive)))
    
        visitor = WrongStringTokenVisitor(default_options, file_tokens=file_tokens)
        visitor.run()
    
>       assert_errors(visitor, [WrongMultilineStringViolation])

tests/test_visitors/test_tokenize/test_primitives/test_string_tokens/test_string_multiline.py:79: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

visitor = <wemake_python_styleguide.visitors.tokenize.primitives.WrongStringTokenVisitor object at 0x7ffacd4dbc50>
errors = [<class 'wemake_python_styleguide.violations.consistency.WrongMultilineStringViolation'>]
ignored_types = None

    def factory(
        visitor: BaseVisitor,
        errors: Sequence[str],
        ignored_types=None,
    ):
        if ignored_types:
            real_errors = [
                error
                for error in visitor.violations
                if not isinstance(error, ignored_types)
            ]
        else:
            real_errors = visitor.violations
    
>       assert len(errors) == len(real_errors)
E       AssertionError: assert 1 == 0
E        +  where 1 = len([<class 'wemake_python_styleguide.violations.consistency.WrongMultilineStringViolation'>])
E        +  and   0 = len([])

tests/test_visitors/conftest.py:31: AssertionError

I added a \n in the primitive to be consistent with the new rule, but something is still wrong and I don't understand why. Do you have a clue @sobolevn ?
Do not hesitate if you want more information on the bug.

@sobolevn
Copy link
Member Author

You can use ignored_types= parameter on assert_errors. This happens when old tests are executed on updated visitor.

Or you can refactor old tests. Both cases are fine.

Mema5 pushed a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 23, 2020
Mema5 pushed a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 23, 2020
Mema5 pushed a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 23, 2020
Mema5 pushed a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 23, 2020
@helpr helpr bot added the pr-available label Feb 23, 2020
Mema5 pushed a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 25, 2020
…edViolation (WPS357). Follows the general structure for documentation.

Relates to original issue wemake-services#1081. Closes #11
sobolevn added a commit that referenced this issue Oct 20, 2020
* feat: added the new violation to be completed, closes #7

* refac: refactor for flake8 consistancy, issue #7

* feat: implementing the new rule. Build yet to be debugged. Issue #9

* test: added a test for violation WPS357. A raw string must contain the '\' 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

* documentation: Added documentation for the violation RawStringNotNeededViolation (WPS357). Follows the general structure for documentation.
Relates to original issue #1081. Closes #11

* fix: Removed an unnecessary single quotation mark in the documentation
relates to #11

* fix: repaired build a priori, issue #9

* fix: adapted test suite to respect the new rule. Temporarily removed some cases to make the test pass. Related to issue #9

* fix: corrected the WPS warnings, issue #9

* fix: enforce the new rule WPS357 in the project itself

* refac: old tests to match the new rule or ignore it, issue #9

* refac: last refactoring. Closes #9

* doc: changelogs for new rule

* test: added a test for wrong case. PR #1178

* fix: minor changes required for PR #1187

* Doc:
* CHANGELOG.md: new header for release 0.15.0
* added contribution
* Related to PR #1178

* test: new tests for rule WPS357. Not passing for escaped quote. PR #1178

* fix: correct type and missing newline. PR #1178

* tests: refactored tests that now all pass. Closes #9. Related to PR #1178

* doc: the previous usage was actually a wrong usage. PR #1178

* refac: remove useless encode().decode()

* merged and solve the conflits. All tests pass

* fix: minor changes for PR #1178

* fix: error linter wrong indentation, related to PR #1178

* fix: wrong error_template in for RawStringNotNeededViolation

* Fixes Ci

* Fixes Ci

Co-authored-by: Mael MADON <mael.madon@polytechnique.edu>
Co-authored-by: fabian.waldner@gmail.com <fabian.waldner@gmail.com>
Co-authored-by: sobolevn <mail@sobolevn.me>
@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
help wanted Extra attention is needed level:starter Good for newcomers pr-available rule request Adding a new rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants