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

CI: Check for whitespaces before class #28489

Merged
merged 2 commits into from
Nov 13, 2019

Conversation

Ayowolet
Copy link
Contributor

@Ayowolet Ayowolet commented Sep 18, 2019

xref #28209, python-sprints/pandas-mentoring#161

Validate that we use

class Foo:
    """
    Doc.
    """
    pass

instead of:

class Bar:

    """
    Doc.
    """
    pass

(note the blank line between class Bar: and the docstring.

@Ayowolet
Copy link
Contributor Author

#28209 @datapythonista I have created the pull request, was a little unsure but does it look okay?

@datapythonista datapythonista added the CI Continuous Integration label Sep 18, 2019
Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Great job @Ayowolet, looks almost perfect, just added couple of comments.

Also, would be great if you can find any class definition in the code, and add the blank line we're trying to detect here. This way, the continuous integration will become red, and we'll see that the check you're adding is working. After we see that it's working, you'll have to revert the blank line, before we can merge.

cc @jbrockmendel

@@ -184,6 +184,11 @@ if [[ -z "$CHECK" || "$CHECK" == "patterns" ]]; then
invgrep -R --include="*.rst" ".. ipython ::" doc/source
RET=$(($RET + $?)) ; echo $MSG "DONE"

# Check for blank lines after the class definition
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this comment, it's already explained in the message in the next line.

@@ -184,6 +184,11 @@ if [[ -z "$CHECK" || "$CHECK" == "patterns" ]]; then
invgrep -R --include="*.rst" ".. ipython ::" doc/source
RET=$(($RET + $?)) ; echo $MSG "DONE"

# Check for blank lines after the class definition
    MSG='Check for extra blank lines after the class definition' ; echo $MSG
    invgrep -R --include="*.py" --include="*.pyx" -E 'class.*:\n\n( )+"""' doc/source/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
    invgrep -R --include="*.py" --include="*.pyx" -E 'class.*:\n\n( )+"""' doc/source/
    invgrep -R --include="*.py" --include="*.pyx" -E 'class.*:\n\n( )+"""' .

The goal here is to check all the Python files in the repo, to standardize classes. The last path doc/source/ is checking only in the documentation directory, where I don't think there are python files, so we want to look in the root instead

@jbrockmendel
Copy link
Member

pending @datapythonista's suggestion about adding-then-reverting a failing case: LGTM

@Ayowolet
Copy link
Contributor Author

Ayowolet commented Sep 19, 2019

@datapythonista when I edit a class and add more spaces, should I commit it to this PR?

@datapythonista
Copy link
Member

Exactly.

The idea is that you add a blank line after the class Whatever: line of any class in our code, and push it. Then we wait for the CI, and if your changes are working, it should be red and report the error. If that happens, you just delete the blank line again, and we merge.

@pep8speaks
Copy link

Hello @Ayowolet! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 37:1: W293 blank line contains whitespace

@datapythonista
Copy link
Member

Looks like your changes are not working. If you go the the CI, in the Checks build, and the step Looking for unwanted patterns, you'll see some errors.

What I'd do is next:

  • Just leave one blank line and not two in what you added last, and remove any space in those blank lines
  • Execute locally ./ci/code_checks.sh patterns
  • Fix the problems in your expression until it reports that file
  • Then commit and push to this branch

Let me know if you need help. Thanks!

@jbrockmendel
Copy link
Member

@Ayowolet can you rebase

@Ayowolet
Copy link
Contributor Author

Ayowolet commented Oct 9, 2019

@jbrockmendel, I'm not sure I understand what you mean by rebase...

@jbrockmendel
Copy link
Member

I'm not sure I understand what you mean by rebase...

Near the bottom of this page there is a gray warning saying "This branch has conflicts that must be resolved". To resolve these, you'll need to run git pull upstream master and fix any merge conflicts that show up.

@Ayowolet
Copy link
Contributor Author

Ayowolet commented Oct 9, 2019

okay, thank you I will do that

@jbrockmendel
Copy link
Member

@Ayowolet can you rebase and see why the check is failing on azure

@Ayowolet
Copy link
Contributor Author

Ayowolet commented Oct 21, 2019 via email

@TomAugspurger
Copy link
Contributor

@Ayowolet the logs have been removed now, but here are a few failures.

 ./pandas/tests/test_algos.py#L37

./pandas/tests/test_algos.py(37,1): error W293: blank line contains whitespace
 ./pandas/tests/test_algos.py#L38

./pandas/tests/test_algos.py(38,5): error E303: too many blank lines (2)

 Build log #L42

Bash exited with code '2'.

I'm not sure what the last one means. You might try running ./ci/code_checks.sh lint locally.

@jbrockmendel
Copy link
Member

I tried the other method you suggested

git pull upstream master didn't work? That should be a fairly light-weight request, so I don't know what would cause that.

The azure page having trouble loading might make more sense, so I'll check that. Looks like the linting is failing because of excess whitespace on tests.test_algos lines 37 and 38

@WillAyd
Copy link
Member

WillAyd commented Nov 7, 2019

I think this is OK. @Ayowolet can you fix the class docstring again so CI turns green?

@jbrockmendel
Copy link
Member

@Ayowolet did the trouble with rebasing get resolved?

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm

I think the regex could be improved, but this is surely good enough for now, and we can improve it if we ever get false positives.

@jreback jreback added this to the 1.0 milestone Nov 13, 2019
@jreback jreback added the Code Style Code style, linting, code_checks label Nov 13, 2019
@jreback jreback merged commit a562c22 into pandas-dev:master Nov 13, 2019
@jreback
Copy link
Contributor

jreback commented Nov 13, 2019

thanks @Ayowolet

Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants