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

Add Python imports sorter to CI and supply-chain review #48

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

ajnelson-nist
Copy link
Contributor

This PR adds and applies Python code-formatting to CI and adds a supply chain review step to keep the pinned formatter references "reasonably" up to date.

The extra logic in the check-supply-chain-pre-commit target will update the version-pin if an update is available, but will not raise a notice unless updating the tool version would induce a change in any of the Python files. (Changes in the Python files are checked with the pinned formatter version in CI.) These lessons were learned between case-utils PR 37 and CDO-Shapes-Example PR 1.

The new make target that creates a Python virtual environment, dedicated to the user's current repository clone, does this in order to avoid a Git logistical issue if make clean is run. (Described further in case-utils PR 37.)

Tools noted in the referenced PRs will be added in future PRs. isort is added as the first tool in order to avoid code conflicts with in-flight non-formatting revisions in other PRs.

Impact of merging this PR will be:

  • Code formatting will follow a consistent style---currently scoped to imports-sorting---checked as part of CI.
  • Contributors (who check CONTRIBUTE.md) should see instructions to run make, or otherwise install pre-commit in order to handle now-delegated code formatting decisions.
  • Anyone who commits Python changes that would be stylistically altered by the auto-formatting will trigger a CI failure when the PR is filed. This is, in my opinion, an easy-enough fix for a project maintainer. Running make ; git commit --amend; git commit -m "Format" in the user's branch should provide a pushable patch atop the submission (noting the middle command fails, per pre-commit's execution style).

References

Disclaimer

Participation by NIST in the creation of the documentation of mentioned software is not intended to imply a recommendation or endorsement by the National Institute of Standards and Technology, nor is it intended to imply that any specific software is necessarily the best available for the purpose.

This patch applies effects of the following patch that adds `isort` to CI.

The patch-ordering is because adding to CI first would cause CI to fail
on finding files need to be re-formatted by `pre-commit`.  This also
separates manual changes from automated changes.

Disclaimer:
Participation by NIST in the creation of the documentation of mentioned
software is not intended to imply a recommendation or endorsement by the
National Institute of Standards and Technology, nor is it intended to
imply that any specific software is necessarily the best available for
the purpose.

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
This patch draws on Cyber Domain Ontology deployments from the noted
PRs.

References:
* casework/CASE-Utilities-Python#37
* Cyber-Domain-Ontology/CDO-Shapes-Example#1

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
Text adapted from contributions to `case-utils` by @kchason.

Disclaimer:
Participation by NIST in the creation of the documentation of mentioned
software is not intended to imply a recommendation or endorsement by the
National Institute of Standards and Technology, nor is it intended to
imply that any specific software is necessarily the best available for
the purpose.

References:
* casework/CASE-Utilities-Python#37

Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>
@ajnelson-nist
Copy link
Contributor Author

This PR is ready for review and merge.

@ajnelson-nist ajnelson-nist marked this pull request as ready for review June 21, 2024 21:39
Copy link
Contributor

@simsong simsong left a comment

Choose a reason for hiding this comment

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

Wow. A staggering number of changes. isort really has fun with this.
If you are going to do this, why not just go full blown autopep8 or black?

@ajnelson-nist
Copy link
Contributor Author

Wow. A staggering number of changes. isort really has fun with this. If you are going to do this, why not just go full blown autopep8 or black?

I tried separating the changes into commits that were manual vs. commits that were automated effects. Sorry, I forgot to mention the right tab to look at first was the "Commits" tab, not the "Files changed" tab.

I put this PR in first because it's adding pre-commit and pre-commit maintenance logistics, and isort is the least-code-touching reviewer among the ones I was going to import from the CASE repositories' practices. I wanted this PR to check in on whether this pre-commit semi-automated maintenance practice is OK. I've found running pre-commit autoupdate nightly and keeping the pinned version up to date is a little too frequent for my maintenance preferences. Updating the pinned version whenever it changes code keeps pre-commit from mixing in unrelated effects to incoming contributions.

I will add Black in another PR after closing PR 45 tomorrow morning. I'm pretty sure Black would cause a few code conflicts with that PR while it's in-flight.

I hadn't heard of autopep8. The CASE repositories happen to use flake8. I'll try those as I add Black.

@simsong
Copy link
Contributor

simsong commented Jun 24, 2024 via email

@simsong simsong merged commit 68fef71 into main Jun 24, 2024
8 checks passed
@simsong simsong deleted the add_isort_via_pre_commit branch June 24, 2024 16:48
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