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

Detect getters and setters #1054

Closed
sobolevn opened this issue Dec 4, 2019 · 12 comments · Fixed by #1185
Closed

Detect getters and setters #1054

sobolevn opened this issue Dec 4, 2019 · 12 comments · Fixed by #1185
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

sobolevn commented Dec 4, 2019

Rule request

Thesis

We can inspect class and instance methods to find these pattern: if class contains two methods like get_X and set_X we should raise UnpythonicGetterSetterViolation

We should also detect cases like:

  • class has instance or class level attribute x or _x or __x
  • class has method called get_x or set_x

It is oop violation.

Reasoning

Because python does not need this abstraction:

  • Either use @property
  • Or just make the property public and change it
@sobolevn sobolevn added help wanted Extra attention is needed level:starter Good for newcomers rule request Adding a new rule labels Dec 4, 2019
@sobolevn sobolevn added this to the Version 0.15 milestone Dec 4, 2019
@adrwes
Copy link
Contributor

adrwes commented Feb 21, 2020

Hi, can I take the issue?

@sobolevn
Copy link
Member Author

Sure! @Gwin73 thanks a lot!

Feel free to ask any questions.

@sobolevn
Copy link
Member Author

@Gwin73 I have updated the task description. Please, take a look 🙂

adrwes added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 21, 2020
@ruwaid4
Copy link
Contributor

ruwaid4 commented Feb 22, 2020

Hello, i am part of the same group as @Gwin73 who is working on this issue. Just to clarify some things, should it be:

  • class has instance or class level attribute x or _x or __x
    AND
  • class has method called get_x or set_x?

So when both cases are satisfied we should raise UnpythonicGetterSetterViolation or should we do so when either case is satisfied?

@sobolevn
Copy link
Member Author

When either case is satisfied 🙂 (logical or)

@ruwaid4
Copy link
Contributor

ruwaid4 commented Feb 22, 2020

Thanks for the quick answer! So this would mean no instance or class level attributes are allowed since x matches anything?

@sobolevn
Copy link
Member Author

I mean x is an abstract field name. It can be anything. Any name.

@adrwes
Copy link
Contributor

adrwes commented Feb 22, 2020

Wait I think there must be some misunderstanding here. Instance or class level attributes x or _x or __x must be allowed right, as long as they are "public" and have no corresponding get_x or set_x methods?

@adrwes
Copy link
Contributor

adrwes commented Feb 22, 2020

Only allowing properties and NO attributes seems a bit weird.

@sobolevn
Copy link
Member Author

sobolevn commented Feb 22, 2020

Sorry, that I am not able to express myself clear enough 🙂

This rule only checks method names. We need to detect patterns like get_${existing_attribute_name} and set_${existing_attribute_name} and raise violations when any of these methods are found.

Where ${existing_attribute_name} can be public, protected, or private name of instance or class level attribute.

@ruwaid4
Copy link
Contributor

ruwaid4 commented Feb 22, 2020

So it should be:

  • class has instance or class level attribute x or _x or __x
    AND
  • class has method called get_x or set_x

Logical AND

@sobolevn
Copy link
Member Author

Yes 👍

ruwaid4 pushed a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 24, 2020
ruwaid4 pushed a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 24, 2020
@helpr helpr bot added the pr-available label Feb 24, 2020
adrwes added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 25, 2020
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
added new test cases for test_nonmatching_attribute_getter_setter, such that different
types of assignments are tested.
relates to #22 and wemake-services#1054
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
refactored test_class_mixed to use stacked test parameters to make it easier to test
many combinations. Relates to #22 and wemake-services#1054
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
added accessor as a new test parameter to ease the creation of
different test combinations for test_nonmatching_attribute_getter_setter and
test_instance_and_class_getter_setter.
Relates to #22 and wemake-services#1054
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
renamed parameters in test_instance_and_class_getter_setter and test_nonmatching_attribute_getter_setter
to ease readability.
relates to #22 and wemake-services#1054
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
added tests that checks that invalid use of class attributes
and getters and setters are prohbited.
Relates to #22, wemake-services#1054
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
added test to ensure that class attributes with
non-matching getters and setters is allowed.
relates to #22, wemake-services#1054
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
added a test for dataclass that uses explicit getters and setters on
an instance attribute. This should be prohibited. Right now the test
is failing, which indicates that something in the implementation needs to be looked at.
Relates to #22, wemake-services#1054
ruwaid4 pushed a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 26, 2020
fwald added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 27, 2020
added a test that checks that correct use of dataclass with
the propert annotation is allowed
relates to #22 and wemake-services#1054
adrwes added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 27, 2020
* test: added a new test that checks that methods named get/set are allowed if not containing the name of a class attribute or instance attribute.
relates to #22

* fix: changed description

* test: added a test for checking that correct use of dataclass with property annotation is valid.
relates to #22 and wemake-services#1054

* test: added test with class attribute and instance getters/setters that should be prohibited.
relates to #22 and wemake-services#1054

* test:
added new test cases for test_nonmatching_attribute_getter_setter, such that different
types of assignments are tested.
relates to #22 and wemake-services#1054

* refactor:
refactored test_class_mixed to use stacked test parameters to make it easier to test
many combinations. Relates to #22 and wemake-services#1054

* refactor:
added accessor as a new test parameter to ease the creation of
different test combinations for test_nonmatching_attribute_getter_setter and
test_instance_and_class_getter_setter.
Relates to #22 and wemake-services#1054

* refactor:
renamed parameters in test_instance_and_class_getter_setter and test_nonmatching_attribute_getter_setter
to ease readability.
relates to #22 and wemake-services#1054

* test:
added tests that checks that invalid use of class attributes
and getters and setters are prohbited.
Relates to #22, wemake-services#1054

* test:
added test to ensure that class attributes with
non-matching getters and setters is allowed.
relates to #22, wemake-services#1054

* test:
added a test for dataclass that uses explicit getters and setters on
an instance attribute. This should be prohibited. Right now the test
is failing, which indicates that something in the implementation needs to be looked at.
Relates to #22, wemake-services#1054

* Feat: add checks for AnnAssign

* Refactor: rename variable ´asd´ to ´attribute´

* Test: Adds 3 tests

* Refactor: Renames some tests

* Refactor: Cleans up the tests

* Fix: test in wrong location

* Feat: seperate class and instance methods

* Fix: some linter errors

* Test: Moves test and readds deleted test

* test:
added a test that checks that correct use of dataclass with
the propert annotation is allowed
relates to #22 and wemake-services#1054

* Fix: linter violations

Co-authored-by: fwald <38955346+fwald@users.noreply.github.com>
Co-authored-by: ruwaid123 <43616385+ruwaid123@users.noreply.github.com>
adrwes added a commit to adrwes/wemake-python-styleguide that referenced this issue Feb 27, 2020
ruwaid4 added a commit to adrwes/wemake-python-styleguide that referenced this issue Mar 7, 2020
@sobolevn sobolevn modified the milestones: Version 0.16, Version 0.15 aka New runtime Oct 20, 2020
sobolevn added a commit that referenced this issue Feb 6, 2021
* Adds test for the issue which now fails

* Refactor:
* Renames methods in test class
* Renames GetterAndSetterViolation to UnpythonicGetterSetterViolation
* Disables test for the new violation for the time being
* Changes formatting to pass the wemake linter

* Doc: documented the new violation UnpythonicGetterSetterViolation, solves #2 (#6)

* Adds improved tests for UnpythonicGetterSetterViolation (#10)

* Test: improves test for UnpythonicGetterSetterViolation to cover more cases from the requirements

* Test:
* Adds test with property getter and setter which should be allowed
* Adds test with "private" class and instance attributes and correspondinding getter or setter

* Test/Format:
* Adds false positive test with module attribute, getter and setter
* Adds test with attribute in parent and getter and setter in child
* Changes formatting to more adhere to the linter

* Test:
Adds 2 false positivity tests that probably fails for the code in the business logic PR

* Bussines logic for unPythonicGetterSetterViolation (#17)

* ADD get_attributes function to logic

* Refactor to use get_attributes in visitors

* Add partial logic code for UnpythonicGetterSetterViolation

* Fix: typo

* Enable tests

* Lower count to correct amount

* Revert "Lower count to correct amount"

This reverts commit 4e6698d.

* Lower count to correct amount

* Feat: New violaton unPythonicGetterSetterViolation(WPS614), closes #3

* Refactor: Change in with startswith()

* Refactor: change regex to lstrip()

Co-authored-by: ruwaid123 <43616385+ruwaid123@users.noreply.github.com>

* Doc: add changes to changelog.md

* Refactor: change variable name nd to sub

* Refactor: better use of any()

* Doc: make requested PR changes

* Test: make requirested PR changes, right know linting fails with I001

* Fix: linter error I001

* Doc: change implementation of incorrect methods to ...

* Refactor: use lstrip() earlier

* Tests: expect @Property to raise violation

* Refacotr: Remove decorator checking

* Issue#22 (#24)

* test: added a new test that checks that methods named get/set are allowed if not containing the name of a class attribute or instance attribute.
relates to #22

* fix: changed description

* test: added a test for checking that correct use of dataclass with property annotation is valid.
relates to #22 and #1054

* test: added test with class attribute and instance getters/setters that should be prohibited.
relates to #22 and #1054

* test:
added new test cases for test_nonmatching_attribute_getter_setter, such that different
types of assignments are tested.
relates to #22 and #1054

* refactor:
refactored test_class_mixed to use stacked test parameters to make it easier to test
many combinations. Relates to #22 and #1054

* refactor:
added accessor as a new test parameter to ease the creation of
different test combinations for test_nonmatching_attribute_getter_setter and
test_instance_and_class_getter_setter.
Relates to #22 and #1054

* refactor:
renamed parameters in test_instance_and_class_getter_setter and test_nonmatching_attribute_getter_setter
to ease readability.
relates to #22 and #1054

* test:
added tests that checks that invalid use of class attributes
and getters and setters are prohbited.
Relates to #22, #1054

* test:
added test to ensure that class attributes with
non-matching getters and setters is allowed.
relates to #22, #1054

* test:
added a test for dataclass that uses explicit getters and setters on
an instance attribute. This should be prohibited. Right now the test
is failing, which indicates that something in the implementation needs to be looked at.
Relates to #22, #1054

* Feat: add checks for AnnAssign

* Refactor: rename variable ´asd´ to ´attribute´

* Test: Adds 3 tests

* Refactor: Renames some tests

* Refactor: Cleans up the tests

* Fix: test in wrong location

* Feat: seperate class and instance methods

* Fix: some linter errors

* Test: Moves test and readds deleted test

* test:
added a test that checks that correct use of dataclass with
the propert annotation is allowed
relates to #22 and #1054

* Fix: linter violations

Co-authored-by: fwald <38955346+fwald@users.noreply.github.com>
Co-authored-by: ruwaid123 <43616385+ruwaid123@users.noreply.github.com>

* Refactor: use update() instead of add()

* Tests: expect classmethod to raise violation

* Revert: to old bussines logic

* Refactor: remove get_all_attributes()

* Test: Refactors tests

* Refactor: rename correct_context to is_correct_context

* Tests: refactor some tests

* Refactor: split get_attributes() into two functions

* Tests: add anti_wps428

* Fix: tests

* Tests: Add test that should not raise violation [Fails]

* Tests: Fix test code indentation

* Refactor: Use new implementation of get_attributes

* Refactor: Change methods to apply for new implentation

* Refactor: Change method to apply for new implementation

* Fix: linter error

Line too long.

* Trigger

* Fixes getters/setters detection logic

* Fixes getters/setters detection logic

* Fixes getters/setters detection logic

* Fixes getters/setters detection logic

* Fixes getters/setters detection logic

* Fixes getters/setters detection logic

Co-authored-by: Gwin73 <adrianwesterberg@hotmail.com>
Co-authored-by: Maël Madon <36968988+Mema5@users.noreply.github.com>
Co-authored-by: ruwaid123 <43616385+ruwaid123@users.noreply.github.com>
Co-authored-by: Ruwaid Louis <ruwaid@kth.se>
Co-authored-by: fwald <38955346+fwald@users.noreply.github.com>
Co-authored-by: sobolevn <mail@sobolevn.me>
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