search

creativecommons / quantifying

quantify the size and diversity of the commons--the collection of works that are openly licensed or in the public domain
MIT License
22 stars 30 forks source link

Use pre-commit CI to automatically fix code style issues #68

Closed Darylgolden closed 3 months ago

Darylgolden commented 5 months ago

Problem

Developers have to manually run black and isort to fix code style issues.

Description

pre-commit ci can be used to automatically fix these issues (rather than just detect them, as is the case with the current workflow). This would run upon each pull request or push.

Implementation

TimidRobot commented 5 months ago

Also see https://github.com/creativecommons/cc-legal-tools-app/blob/main/.pre-commit-config.yaml

naishasinha commented 5 months ago

I would be interested in working on this issue, could I possibly be assigned to it? Thank you!

Edit: just noticed that we don't need to ask permission for this thread, so I have started working on it!

Darylgolden commented 5 months ago

@naishasinha Hi! I'm not sure whether you've already put in much work into this, but I was actually planning on working on it (by the checked checkbox in the implementation section).

naishasinha commented 5 months ago

@naishasinha Hi! I'm not sure whether you've already put in much work into this, but I was actually planning on working on it (by the checked checkbox in the implementation section).

my bad, no worries!

TimidRobot commented 4 months ago

Regarding @nox1134 's questions:

  1. Do we also need to add pre-commit hooks for checking yaml, trailing whitespaces etc? (Not mentioned in the issue however i suggest to enable it)

Yes, the more static analysis, the better, in my opinion.

  1. We can have two approaches:
    1. Using (pre-commit ci) or

Yes, this manual option plus documentation on actually setting it up as a pre-commit hook will help developers locally.

  1. adding pre-commit hook in the github actions workflow to run pre-commit after every PR (I am following the 1st approach suggested in the issue, is this what was intended?)

Yes, and it can replace the existing python_static_analysis.yml workflow, which it supersedes.

nox1134 commented 4 months ago

@TimidRobot I've added more static analysis hooks, github action workflow and documentation for running pre-commit hooks. Could you please review the PR #71 ? It seems like the checks are failing due to static issues like trailing whitespaces, mixed lines etc in the codebase. Do i remove the checks from the pre-commit config or should we fix the files impacted by this issue by running pre-commit over the whole codebase?

TimidRobot commented 4 months ago

@nox1134

Do i remove the checks from the pre-commit config or should we fix the files impacted by this issue by running pre-commit over the whole codebase?

I recommend:

  1. keeping the checks in the config, but commenting them out to disable them
  2. write an issue for each check (or category of check) that is disabled (I can assist, if you'd like)
  3. then people can submit PRs for each issue with the changes per PR minimized
Darylgolden commented 3 months ago

I think this issue should be reopened; the CI does not automatically fix issues but only checks it.

TimidRobot commented 3 months ago

I think this issue should be reopened; the CI does not automatically fix issues but only checks it.

@Darylgolden - My preference is for Continuous Integration (CI) to be report-only. I would like machines to facilitate humans making changes, not machines making changes. (I realize that the pre-commit hook makes changes when installed, but it does so in front of the human that initiated the commit command)

TimidRobot commented 3 months ago

@Darylgolden I do think there's still room for improvement:

Darylgolden commented 3 months ago

@TimidRobot pre-commit CI is primarily designed to be run automatically, and I believe it would have already ran automatically on the repo if the marketplace app was installed (autofixing is the default behavior, but it still would not have ran correctly as seen here because the hooks were not setup correctly). When pre-commit is installed locally, it also automatically modifies files when a commit is made.

However, I believe it's possible to make precommit run only via a command by setting its stage to manual and/or setting autofix_prs to false. Then pre-commit can also run when pre-commit.ci autofix and/or pre-commit run --hook-stage manual is commented on a PR. This can then replace dev/tools.sh.

Darylgolden commented 3 months ago

For the CI, we can either set the pre-commit configuration to have some hooks that run manually and some that run automatically, and have only hooks that don't modify files to run automatically. The devs of pre-commit had explicitly rejected the idea of a check-only mode, so there is no protection against modification if a hook that modifies files is accidentally added to the automatic mode. This would allow us to remove static_analysis.yml entirely as precommit-ci would run automatically on any repository set up with the marketplace extension and the yaml file.

Alternatively, we can keep static_analysis.yml; this would allow us to use the same hooks for checking and manually triggered modifications, and provides safety against files from being automatically modified. For the installation of tools, I believe precommit can handle the installation itself if provided with the URLs of the repos to be installed, like it is in my PR. This prevents needing to specify any installation for static analysis tools in static_analysis.yml.

I think I prefer the second approach, but ultimately agree with pre-commit's philosophy of allowing the tools to automatically modify files. But I'll of course still work on the manual approach if desired.

TimidRobot commented 3 months ago

@Darylgolden -

@TimidRobot pre-commit CI is primarily designed to be run automatically, and I believe it would have already ran automatically on the repo if the marketplace app was installed (autofixing is the default behavior, but it still would not have ran correctly as seen here because the hooks were not setup correctly). When pre-commit is installed locally, it also automatically modifies files when a commit is made.

I don't want our GitHub Actions to modify files (or, more specifically, to commit any modifications).

I'm ok with commit hooks modifying files. They do so under user supervision following a user command.

For the CI, we can either set the pre-commit configuration to have some hooks that run manually and some that run automatically, and have only hooks that don't modify files to run automatically. The devs of pre-commit had explicitly rejected the idea of a check-only mode, so there is no protection against modification if a hook that modifies files is accidentally added to the automatic mode. This would allow us to remove static_analysis.yml entirely as precommit-ci would run automatically on any repository set up with the marketplace extension and the yaml file.

Alternatively, we can keep static_analysis.yml; this would allow us to use the same hooks for checking and manually triggered modifications, and provides safety against files from being automatically modified. For the installation of tools, I believe precommit can handle the installation itself if provided with the URLs of the repos to be installed, like it is in my PR. This prevents needing to specify any installation for static analysis tools in static_analysis.yml.

I think I prefer the second approach, but ultimately agree with pre-commit's philosophy of allowing the tools to automatically modify files. But I'll of course still work on the manual approach if desired.

When pre-commit is initiated directly in a GitHub action, not by pre-commit.ci, it changes files but those files are not committed. This results in a check instead of a modification. For example: creativecommons/cc-legal-tools-app/: .github/workflows/static-analysis.yml. This satisfies the preferences I stated above, and avoids adding yet another service, and avoids maintaining python module versions in multiple places.