Added TooLongVariableNameViolation #291

Casva · 2018-10-24T22:52:15Z

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 #288
Refs #288

Let me know if anything needs to be changed! 😄

coveralls · 2018-10-24T23:23:41Z

Pull Request Test Coverage Report for Build 681

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

Totals
Change from base Build 680:	0.0%
Covered Lines:	1582
Relevant Lines:	1582

💛 - Coveralls

sobolevn

Thanks for starting to work on this rule!

You can continue to implement the business logic for this rule, while we are releasing the 0.3.0 version.

Later, I will come back for the full review and we will merge it into 0.4.0.
Does this sound fine to you?

Casva · 2018-10-26T21:41:58Z

@sobolevn yup sure no problem!

nixphix · 2018-10-28T14:34:31Z

wemake_python_styleguide/violations/naming.py

+
+        # Correct:
+        totalPrice = 100
+        coursesEnrolled = 5


Just nitpicking, PEP8 does not recommend camelCase

Function and Variable Names

Function names should be lowercase, with words separated by underscores as necessary to improve readability.

Variable names follow the same convention as function names.

mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

we may not want the example to promote wrong notion, especially on a style guide :-)

I agree with @nixphix

@nixphix Noted! Just made the changes 👍

sobolevn

Thanks!

@Casva can you please also add a business logic and tests for your rule?

CHANGELOG.md

wemake_python_styleguide/options/config.py

wemake_python_styleguide/violations/naming.py

Casva · 2018-10-31T16:45:02Z

@sobolevn sure not a problem!

I also noticed that the "TooShortVariableNameViolation" rule was changed to "TooShortNameViolation" and checks for modules, variables, attributes, functions, methods, and classes.

Did you want me to change TooLongVariableNameViolation to do the same thing?

sobolevn · 2018-10-31T18:05:22Z

@Casva yes, I have unified logic for variables and modules. It seems reasonable to me, what do you think?

Casva · 2018-11-01T07:31:12Z

@sobolevn makes sense to me.
I'll do the same with the too long rule as well

sobolevn

Awesome work! Thank you.

Just some documentation fixes and we are ready to go!

sobolevn · 2018-11-01T09:03:06Z

wemake_python_styleguide/logics/naming/logical.py

+    False
+
+    """
+    return name != constants.UNUSED_VARIABLE and len(name) > max_length


We do not care about _ here. Since it is only one char long.

sobolevn · 2018-11-01T09:03:21Z

tests/test_visitors/test_filenames/test_module/test_module_name_length.py

+    visitor.run()
+
+    assert_errors(visitor, [TooLongNameViolation])
+    assert_error_text(visitor, filename.replace('.py', ''))


Missing new line at the end of the file.

sobolevn · 2018-11-01T09:03:41Z

wemake_python_styleguide/options/config.py

@@ -43,6 +43,9 @@ class Configuration(object):
    - ``min-name-length`` - minimum number of chars to define a valid
      variable and module name, defaults to
      :str:`wemake_python_styleguide.options.defaults.MIN_NAME_LENGTH`
+    - ``max-name-length`` - maximum number of chars to define a valid
+      variable name, defaults to


variable and module name

sobolevn · 2018-11-01T09:03:57Z

wemake_python_styleguide/options/config.py

+        _Option(
+            '--max-name-length',
+            defaults.MAX_NAME_LENGTH,
+            'Maximum possible length of the variable name.',


variable and module name

sobolevn · 2018-11-01T09:04:14Z

wemake_python_styleguide/options/defaults.py

@@ -21,6 +21,9 @@
 #: Minimum variable's name length.
 MIN_NAME_LENGTH: Final = 2

+#: Maximum variable's name length:


variable and module name length

sobolevn · 2018-11-01T09:04:34Z

wemake_python_styleguide/violations/naming.py

+@final
+class TooLongNameViolation(ASTViolation):
+    """
+    Forbids to have too long variable or module names.


variable and module names

sobolevn · 2018-11-01T09:05:20Z

wemake_python_styleguide/violations/naming.py

+
+    .. versionadded:: 0.4.0
+
+    Note:


We do not use Note anymore, see this commit: e428aab

sobolevn · 2018-11-01T09:05:32Z

wemake_python_styleguide/violations/naming.py

+
+    """
+
+    #: Error message shown to the user.


We do not use this comment anymore, see this commit e428aab

Casva · 2018-11-02T03:30:21Z

@sobolevn Alright! Everything should be good to go now! 😄

Casva · 2018-11-02T04:05:19Z

There's a lot of names in the repo that are longer than the default maximum length (29 characters).
Should I change the default length to 40 characters instead?

sobolevn

Just a single typo left!

sobolevn · 2018-11-02T07:05:57Z

wemake_python_styleguide/logics/naming/logical.py

there's an error here. please, remove name !=

Casva · 2018-11-02T19:12:37Z

@sobolevn Aand done! 👍

sobolevn · 2018-11-02T20:07:41Z

@Casva yes, please change it to 40 for now.
I will play with this value afterwards.

Also, please take care of some build issues.
It may be easier to run tests to see what's wrong.

Casva · 2018-11-03T09:28:55Z

@sobolevn after some mass cleaning up it's almost perfect!
There are a few things I wanted to point out though:

I had to change the MAX_NAME_LENGTH to 55 to accommodate the longest name in the repo (which i believe was 53 characters)
I also had to increase to max-complexity by 1 since I added a check for max_name_length under the _check_name method
under logical.py the line exceeds the allowed maximum length but backslash is not allowed for line breaks, is there another option I could use?

Thanks!

sobolevn · 2018-11-03T10:05:18Z

setup.cfg

@@ -11,7 +11,7 @@ branch = True


 [flake8]
-max-complexity = 6
+max-complexity = 7


Please, do not change this value.

sobolevn · 2018-11-03T10:09:13Z

@Casva thanks for your work! We are almost there!

Yes, seems fine. I will double check it later
Please, do not do that. Instead we need to refactor the code if it gets too complex. That's the point why we use this check
You can just ignore the doctest for this function, no big deal

👍

Casva · 2018-11-03T11:44:01Z

@sobolevn Got it, I changed max-complexity back to its original value

I'm at a loss for refactoring: I've tried grouping the length testers into another method and incorporating that in the original _check_name method but that exceeds to max_methods for the class.
Aaand making a brand new class to contain some of the tests doesn't seem like the best idea either. 😢

sobolevn · 2018-11-03T16:33:48Z

@Casva ok, leave this method as noqa for now.
I will handle that, since I am currently refactoring a lot of modules anyway.

P.S. Have you tried this project on your own codebase? What are the results? Do you find it useful? Any feedback is appreciated 🙂

Casva · 2018-11-04T10:03:40Z

@sobolevn alright done! Thanks for all your patience!

And yes I tried using it and I realized how poorly I code 😆
I'm still in school at the moment and this was a great tool to give me an idea of best practices to use while coding 👍

sobolevn · 2018-11-04T18:00:55Z

@Casva it was still failing.

Can you please rebase your changes and fix the tests?
I have implemented a new logic for WrongNameVisitor. So, it would be easy.
I have also refactored how tests are located.

Casva · 2018-11-07T10:00:09Z

Sorry for the delay @sobolevn, been busy with school work.
Everything should be good to go!

Let me know if there's anything else you need changed! 👍

sobolevn · 2018-11-09T12:17:04Z

@Casva can you please resolve the merge conflicts once again please?
And I will merge it asap.

Thanks!

…gVariableNameViolation rule Updated CHANGELOG.md

…nd associated tests Fixed config MAX_VARIABLE_LENGTH -> MAX_NAME_LENGTH, updated types.py to contain max_name_length added rule integration into noqa and test_noqa Fixed documentation (mainly variable -> variable and module) Forgot extra line at the end of logical.py Reordered imports to match tests Fixed line 188 in logical.py Changed default max-length to 40, changed to tests to reflect this as well

ASTViolation -> MaybeASTViolation in naming.py | fixed multi-line string error (added \) in logical.py | other syntax fixes Removed WrongModuleNamePatternViolation in test_module_name_length.py on line 56 | replaced backslashe line break with indent)

…_check_name under naming.py Removed WrongModuleNamePatternViolation in test_module_name_length.py on line 56 | replaced backslashe line break with indent) Changed max-complexity from 6 to 7 to accomodate new if statement in _check_name under naming.py

Refactored _check_name to include new function _check_name_length; reduces complexity Erased variable assignment in _check_name instead Reverted _check_name back to original

Also removed test in test_naming until _check_name is refactored Removed instances of previous TooLongNameViolation tests in naming.py and test_naming.py

Fixed imports Fixed whitspace issue Workaround for line limit a a a a

Casva · 2018-11-10T09:34:11Z

@sobolevn rebased and fixed!

sobolevn · 2018-11-10T10:16:50Z

@Casva you have made an amazing job! Thank you very much!

sobolevn requested changes Oct 25, 2018

View reviewed changes

sobolevn added the on hold label Oct 25, 2018

nixphix reviewed Oct 30, 2018

View reviewed changes

sobolevn requested changes Oct 31, 2018

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

wemake_python_styleguide/options/config.py Outdated Show resolved Hide resolved

wemake_python_styleguide/violations/naming.py Show resolved Hide resolved

wemake_python_styleguide/violations/naming.py Outdated Show resolved Hide resolved

Casva force-pushed the issue-288 branch from 35036e3 to 7e75239 Compare November 1, 2018 06:49

sobolevn removed on hold labels Nov 1, 2018

sobolevn requested changes Nov 1, 2018

View reviewed changes

sobolevn requested changes Nov 2, 2018

View reviewed changes

Casva force-pushed the issue-288 branch from 50addd5 to 0fc0d05 Compare November 3, 2018 09:13

sobolevn reviewed Nov 3, 2018

View reviewed changes

setup.cfg Outdated

@@ -11,7 +11,7 @@ branch = True

[flake8]

max-complexity = 6

max-complexity = 7

Copy link

Member

sobolevn Nov 3, 2018

Choose a reason for hiding this comment

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

Please, do not change this value.

Casva force-pushed the issue-288 branch from ff4b351 to c552476 Compare November 3, 2018 11:33

Casva force-pushed the issue-288 branch from 1f8bc65 to eb1ba18 Compare November 4, 2018 09:16

Casva force-pushed the issue-288 branch 2 times, most recently from 9bcaf66 to 65f5ef8 Compare November 7, 2018 08:48

Casva force-pushed the issue-288 branch 6 times, most recently from 7fe9b05 to 3d3bb8a Compare November 7, 2018 09:37

Casva added 10 commits November 10, 2018 04:26

Issue-288: Changed config.py, defautls.py and naming.py to add TooLon…

9d96943

…gVariableNameViolation rule Updated CHANGELOG.md

Left 178 181 and 184 as one line for now in logical.py

9f21d20

Refactored _check_name to include new function _check_name_length; reduces complexity Erased variable assignment in _check_name instead Reverted _check_name back to original

Removed check for long name in naming.py; will be refactored later

acbe2b6

Also removed test in test_naming until _check_name is refactored Removed instances of previous TooLongNameViolation tests in naming.py and test_naming.py

Removed extra blank line

96c3b60

Rebased branch and made appropriate changes

4a934b2

Fixed imports Fixed whitspace issue Workaround for line limit a a a a

Restructured tests in logical.py to fit line size

b28a2e0

Renumbered noqa to 118, previous function was deleted

6f838e8

Casva force-pushed the issue-288 branch from 3d232ad to 6f838e8 Compare November 10, 2018 09:29

sobolevn approved these changes Nov 10, 2018

View reviewed changes

sobolevn merged commit 22ea20c into wemake-services:master Nov 10, 2018

Added TooLongVariableNameViolation #291

Added TooLongVariableNameViolation #291

Conversation

Casva commented Oct 24, 2018 • edited by sobolevn

I have made things!

Checklist

Related issues

coveralls commented Oct 24, 2018 • edited

Pull Request Test Coverage Report for Build 681

💛 - Coveralls

sobolevn left a comment

Choose a reason for hiding this comment

Casva commented Oct 26, 2018

Choose a reason for hiding this comment

Function and Variable Names

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolevn left a comment

Choose a reason for hiding this comment

Casva commented Oct 31, 2018

sobolevn commented Oct 31, 2018

Casva commented Nov 1, 2018

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Casva commented Nov 2, 2018

Casva commented Nov 2, 2018

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Casva commented Nov 2, 2018

sobolevn commented Nov 2, 2018

Casva commented Nov 3, 2018

Choose a reason for hiding this comment

sobolevn commented Nov 3, 2018

Casva commented Nov 3, 2018

sobolevn commented Nov 3, 2018

Casva commented Nov 4, 2018

sobolevn commented Nov 4, 2018

Casva commented Nov 7, 2018

sobolevn commented Nov 9, 2018

Casva commented Nov 10, 2018

sobolevn commented Nov 10, 2018 • edited

Casva commented Oct 24, 2018 •

edited by sobolevn

coveralls commented Oct 24, 2018 •

edited

sobolevn commented Nov 10, 2018 •

edited