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

Simplify, unify and document pre_push, CIs and pre-commit hooks to all use consistent tools, versions and config #1784

Merged
merged 4 commits into from
Sep 17, 2021

Conversation

CAM-Gerlach
Copy link
Contributor

Followup to PR #1774 as requested by @LilSpazJoekp

Feature Summary and Justification

Per my comment on #1774 :

Given that all the now-duplicate dep installation and lint runner commands lint-multi-os job in your CI workflow (aside from the Sphinx build) is now also covered here, I would suggest replacing all that redundancy with the pre-commit Github action which will install and run them through pre-commit, and tweak your caching as needed. This ensures the same checks are run locally as in the CI (right now some of the invocations are different, because they were changed here but not there, and the versions may drift out of sync), keeps things DRY and avoids doubling the maintenance burden, with corresponding issues if things get out of sync.

Similarly, now that you've done this, I suggest not further duplicating the invocations and versions again in pre_push.py and the setup.py lint extra respectively, and instead replacing all that with a single call to pre-commit, i.e. pre-commit run or pre-commit run --all-files. In all, this triples the maintenance burden of keeping versions up to date and tweaking invocations (particularly since pre-commit handles the former and much of the latter for you), again ensures consistent behavior between the script and the hooks, and brings the benefits of pre-commit's automated, platform-independent installation, updating, output reporting and more.

Significant changes:

  • Simply run the linting suite with pre-commit in the pre_push.py script instead of redundant manual invocations of each linter.
  • Also in pre_push.py, use TemporaryDirectory instead of the legacy tempfile API to further simplify the code and clean up the temp dir automatically
  • Eliminate all the duplicate linter invocations in the Github Actions CI workflow and just use the pre-commit action to run them instead, with consistent tools, invocations and versions
  • Remove the now-unused lint dependencies that are installed and managed through pre-commit and not ever run directly from the lint extra and move pre-commit there, where it belongs
  • Document how to install/run the pre-commit hooks in both (...) contributing guides, and streamline and update the guidance for the pre_push.py script; also update/remove a few other related obsolete mentions
  • Update hook versions, including fixing a hard crash due to missing deps ( Add appdirs dependency. LilSpazJoekp/docstrfmt#30 ) that went undetected due to running different tools, versions and config via pre-commit, pre_push and CIs (as fixed in this PR)
  • Fix an apparently unnecessary pinned Python version for black and flynt that results in a hard crash on any system without that particular version installed, which again went undetected due to the above
  • Improve docstrfmt runtime by nearly 3x (1 min 20 s to 30 s) by avoiding double-parallelization by both pre-commit and the tool itself

Also resulted in two PRs to fix further issues discovered with docstrfmt:

Thanks!

Copy link
Member

@LilSpazJoekp LilSpazJoekp left a comment

Choose a reason for hiding this comment

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

Looks great! I have just a few suggestions.

and the pre-commit hooks, which ensure any new code conforms to PRAW's
quality and style guidelines. To do so, install the linting dependencies
with `pip install praw[lint]`, then by the hooks with `pre-commit install`.
They will now run automatically every time you commit.
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment about if there is things that are changed with pre-commit that the commit will fail and all that is needed to do is to commit again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I was thinking of adding that, but wasn't sure how far I should go into the details. I'll add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Let me know what you think.

.github/workflows/ci.yml Show resolved Hide resolved
pre_push.py Show resolved Hide resolved
docs/package_info/contributing.rst Outdated Show resolved Hide resolved
@LilSpazJoekp
Copy link
Member

Would you mind taking a look at this as well? I added a workflow to automatically update the versions but it failed. Not sure why prawcore needs to be installed to just update the versions.

Thanks!

@CAM-Gerlach
Copy link
Contributor Author

CAM-Gerlach commented Sep 15, 2021

Would you mind taking a look at this as well? I added a workflow to automatically update the versions but it failed. Not sure why prawcore needs to be installed to just update the versions.

@LilSpazJoekp Sure. The action re-runs pre-commit on all files after updating the hooks to ensure that none of the updates introduced new checks that fail, as can often happen when new rules are added to linters or existing ones are modified. This allows you to handle them with the commit that introduces them. Otherwise, they will start failing on CI with the next unrelated PR, or with locally for any developer who touches the affected files, which is not a good developer UX and allows new issues to linger in the codebase until caught later. This includes running your check_documentation script as a local hook, which in turn is not purely static but imports praw dynamically via some (rather aweful) sys.path hacks. PRAW then imports prawcore, which evidently is not installed by default into the local env by that workflow. You can see the offending call chain from the check output here:

Traceback (most recent call last):
  [SNIP]
  File "/home/runner/work/praw/praw/tools/check_documentation.py", line 10, in <module>
    from praw.models.reddit.base import RedditBase  # noqa: E402
  File "/home/runner/work/praw/praw/praw/__init__.py", line 13, in <module>
    from .reddit import Reddit  # NOQA
  File "/home/runner/work/praw/praw/praw/reddit.py", line 23, in <module>
    from prawcore import (
ModuleNotFoundError: No module named 'prawcore'

So, to resolve this, you could either:

  • Mock the imports to prawcore, and possibly other dependencies (which may require running in an isolated environment to expose) in check_documentation.py (allows running the script standalone without environment dependencies, but will take non-trivial effort for a developer helper script, and might not even be advisable if the dependencies are truly needed)
  • My recommendation, simply do pip install .[dev] as a step in your workflow, like the other jobs do.

@CAM-Gerlach
Copy link
Contributor Author

CAM-Gerlach commented Sep 17, 2021

Looks like merging #1789 predictably created a conflict with this PR; I'll rebase, fix and confirm it doesn't break anything. Sorry for the delay on this that would otherwise have avoided that.

@LilSpazJoekp LilSpazJoekp merged commit 58f0207 into praw-dev:master Sep 17, 2021
@LilSpazJoekp
Copy link
Member

Thank you for helping with this! 🎇

@CAM-Gerlach
Copy link
Contributor Author

Thanks for your support, review and feedback @LilSpazJoekp !

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.

2 participants