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

Pin pylint to 1.7.5 and fix docstring styling #3408

Merged
merged 7 commits into from
Dec 18, 2017

Conversation

humitos
Copy link
Member

@humitos humitos commented Dec 15, 2017

All the test should just pass with this new configuration.

There are a couple of PR that are failing just because of this.

For reviewers: check all the commits but ignore this one at first 4d10f2e (since it's a run of docformatter that produces many changes). I tried to explain the changes the best I could.

# AttributeError: 'NoneType' object has no attribute 'pattern
pylint==1.7.5

prospector
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't prospector also be pinned?

Copy link
Member Author

Choose a reason for hiding this comment

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

"Maybe".

Read my comment below in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm torn on whether we pin these or not. We are fighting broken testing dependencies at least twice a month now. Pinning does help avoid this, but I don't think there is a strong reason to pin otherwise.

@humitos
Copy link
Member Author

humitos commented Dec 16, 2017

Well, it doesn't fail because of the Python exception when running prospector... but for some reason it runs twice prospector commands (as it was doing before) but now it seems that the --profile=prospector is working as it should (checking things with more strictness) and fails as it should.

So, I think we should remove the --die-on-tool-error since we want to check this but we don't want the test to fail.

If I pin the prospector as it was (0.12.5) it doesn't fail, but I think that it doesn't work as it should be either.

@humitos
Copy link
Member Author

humitos commented Dec 16, 2017

I missunderstood the --die-on-tool-error meaning. We don't want to remove that, we want the command to not make travis fail the test.

@@ -1,8 +1,13 @@
-r pip.txt
maxcdn
astroid
pylint
# prospector==0.12.6 currently has issues with pydocstyle
Copy link
Member Author

Choose a reason for hiding this comment

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

I unppined this because I didn't find any error related with pydocstyle. Besides, I think the new version works as it should.

@humitos
Copy link
Member Author

humitos commented Dec 16, 2017

Most of the errors are

D213 / Multi-line docstring summary should start at the second line

that can be fixed by our docformatter command.

prospector \
  --profile-path=/home/humitos/rtfd/code/readthedocs.org \
  --profile=prospector --die-on-tool-error \
  | \
  grep -e "^[a-z] \
  > \
  list-files.txt

for x in `cat list-files.txt`; do \
  docformatter \
    --wrap-summaries=80 \
    --wrap-descriptions=80 \
    --pre-summary-newline \
    --no-blank \
    --in-place readthedocs/$x\
; done
The sections as we use them are useful for rst style.
@@ -35,6 +35,15 @@ pep257:
- D105
- D211
- D104
- D212 # Multi-line docstring summary should start at the first line
Copy link
Member Author

Choose a reason for hiding this comment

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

D213 and D212 are incompatible. We use D213

See: http://pep257.readthedocs.io/en/latest/error_codes.html

@@ -35,6 +35,15 @@ pep257:
- D105
- D211
- D104
- D212 # Multi-line docstring summary should start at the first line
- D107 # Missing docstring in __init__
Copy link
Member Author

Choose a reason for hiding this comment

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

We usually put the docstring at the class level.

(I personally prefer in the __init__, but... it just a preference)

@@ -35,6 +35,15 @@ pep257:
- D105
- D211
- D104
- D212 # Multi-line docstring summary should start at the first line
- D107 # Missing docstring in __init__
- D106 # Missing docstring in public nested class
Copy link
Member Author

Choose a reason for hiding this comment

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

Ignore this one because uses of class Meta: which doesn't have docstrings.

- D406 # Section name should end with a newline ('Examples', not 'Examples::')
- D407 # Missing dashed underline after section ('Examples')
- D412 # No blank lines allowed between a section header and its content ('Examples')
- D413 # Missing blank line after last section ('Examples')
Copy link
Member Author

Choose a reason for hiding this comment

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

Ignore all of them because doesn't allow us to use rst like

Here you have a couple of examples::

  one
  two
  three

inside the docstring.

@humitos humitos changed the title Fix pytlint to 1.7.5 since 1.8.0 has problems with pylint-django Pin pytlint to 1.7.5 and fix docstring styling Dec 16, 2017
@humitos humitos changed the title Pin pytlint to 1.7.5 and fix docstring styling Pin pylint to 1.7.5 and fix docstring styling Dec 16, 2017
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@ericholscher ericholscher merged commit b7434c0 into master Dec 18, 2017
@stsewd stsewd deleted the humitos/tox/fix-lint branch August 15, 2018 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants