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

Expand on and restructure pre_push.py #1157

Merged
merged 5 commits into from
Dec 22, 2019
Merged

Expand on and restructure pre_push.py #1157

merged 5 commits into from
Dec 22, 2019

Conversation

PythonCoderAS
Copy link
Contributor

I feel that there is no reason for pre_push.py to not run the unit tests, so I added in a command to run the unit tests.

I also moved around imports and changed the return of the main function to be the int of the opposite of the value of success (True=0 and False=1).

pre_push.py Outdated Show resolved Hide resolved
pre_push.py Show resolved Hide resolved
pre_push.py Show resolved Hide resolved
pre_push.py Outdated Show resolved Hide resolved
@jarhill0
Copy link
Contributor

I'm just weighing in with my personal opinion, so don't take this as a PR review.

I feel that there is no reason for pre_push.py to not run the unit tests

Could this be optional? Maybe via a command-line option like python3 pre_push.py tests? My reason is that, when I'm developing for PRAW, I usually run tests as I develop them, and once they pass I use pre_push.py for style/docs checks. So in my own use-case, running the tests again would be redundant.

Or, alternatively, could there be an option to do style checks only? Such as python3 pre_push.py style?

@PythonCoderAS
Copy link
Contributor Author

I'm just weighing in with my personal opinion, so don't take this as a PR review.

I feel that there is no reason for pre_push.py to not run the unit tests

Could this be optional? Maybe via a command-line option like python3 pre_push.py tests? My reason is that, when I'm developing for PRAW, I usually run tests as I develop them, and once they pass I use pre_push.py for style/docs checks. So in my own use-case, running the tests again would be redundant.

Or, alternatively, could there be an option to do style checks only? Such as python3 pre_push.py style?

usage: pre_push.py [-h] [-n] [-u] [-a]

Run static and/or unit-tests

optional arguments:
  -h, --help            show this help message and exit
  -n, --unstatic        Do not run static tests
                        (black/flake8/pydocstyle/sphinx-build)
  -u, --unit-tests, --unit
                        Run the unit tests
  -a, --all             Run all of the tests (static and unit). Overrides the
                        unstatic argument.

This is the solution. I understand having -a is redundant, but if I was new I would expect there to be an all option

Copy link
Member

@bboe bboe left a comment

Choose a reason for hiding this comment

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

A few minor suggestions, otherwise I really like this improvement. Great work!

pre_push.py Outdated
@@ -1,9 +1,14 @@
#!/usr/bin/env python3
"""Run static analysis on the project."""

import sys

Copy link
Member

Choose a reason for hiding this comment

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

There needn't be this blank line here. Also argparse should be just before import sys.

pre_push.py Outdated
where any failed tests cause pre_push.py to fail.
"""
curdir = path.abspath(path.join(__file__, ".."))
return do_process([sys.executable, curdir + "/setup.py", "test"])
Copy link
Member

Choose a reason for hiding this comment

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

Can you use "".format here rather than string concatenation? I'm actually more partial to f-strings now, however, python 3.5 doesn't support them, and it's not EOL until September 2020.

@bboe bboe merged commit 3b7e45d into praw-dev:master Dec 22, 2019
@bboe
Copy link
Member

bboe commented Dec 22, 2019

Thanks! 🎆

@PythonCoderAS PythonCoderAS deleted the patch-2 branch December 24, 2019 02:30
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.

3 participants