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 #1054 - unPythonicGetterSetterViolation #1185

Merged
merged 46 commits into from
Feb 6, 2021

Conversation

ruwaid4
Copy link
Contributor

@ruwaid4 ruwaid4 commented Feb 24, 2020

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

🙏 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.

adrwes and others added 9 commits February 21, 2020 20:52
* 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
* 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
* 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
Adds 2 false positivity tests that probably fails for the code in the business logic PR
* 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>
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.

Awesome work! We need to polish it before I can get this merged.

There are also tests to write. Check out how we test our visitors inside tests/test_visitors

tests/fixtures/noqa/noqa.py Outdated Show resolved Hide resolved
tests/fixtures/noqa/noqa.py Outdated Show resolved Hide resolved
tests/test_checker/test_noqa.py Outdated Show resolved Hide resolved
wemake_python_styleguide/logic/tree/classes.py Outdated Show resolved Hide resolved
wemake_python_styleguide/logic/tree/functions.py Outdated Show resolved Hide resolved
wemake_python_styleguide/logic/tree/functions.py Outdated Show resolved Hide resolved
wemake_python_styleguide/violations/oop.py Outdated Show resolved Hide resolved
wemake_python_styleguide/violations/oop.py Outdated Show resolved Hide resolved
wemake_python_styleguide/violations/oop.py Outdated Show resolved Hide resolved
wemake_python_styleguide/violations/oop.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Feb 25, 2020

Pull Request Test Coverage Report for Build 2789

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

Totals Coverage Status
Change from base Build 2771: 0.0%
Covered Lines: 5304
Relevant Lines: 5304

💛 - Coveralls

@ruwaid4
Copy link
Contributor Author

ruwaid4 commented Feb 25, 2020

@sobolevn we have now made the neccesary changes.

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.

Awesome work on such a complex feature!

We need to add more tests (since, again, it is a complex feature) and we are ready to go. 🚀

ruwaid123 and others added 4 commits February 26, 2020 12:32
* 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>
@ruwaid4
Copy link
Contributor Author

ruwaid4 commented Feb 27, 2020

@sobolevn Here is our progress.

@ruwaid4
Copy link
Contributor Author

ruwaid4 commented Feb 28, 2020

@sobolevn You are a great maintaner, thank you for the feedback! Awesome project!

@sobolevn sobolevn added this to the Version 0.15 milestone Mar 9, 2020
@sobolevn sobolevn modified the milestones: Version 0.16, Version 0.15 aka New runtime Oct 20, 2020
@codecov
Copy link

codecov bot commented Feb 6, 2021

Codecov Report

Merging #1185 (f3c763d) into master (c56d8a7) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1185   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          107       107           
  Lines         5958      5983   +25     
  Branches      1330      1338    +8     
=========================================
+ Hits          5958      5983   +25     
Impacted Files Coverage Δ
wemake_python_styleguide/logic/tree/classes.py 100.00% <100.00%> (ø)
wemake_python_styleguide/violations/oop.py 100.00% <100.00%> (ø)
wemake_python_styleguide/visitors/ast/classes.py 100.00% <100.00%> (ø)
...thon_styleguide/visitors/ast/complexity/classes.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 c56d8a7...f3c763d. Read the comment docs.

@sobolevn sobolevn merged commit 9b3e342 into wemake-services:master Feb 6, 2021
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.

Detect getters and setters
8 participants