Use pre-commit to run linters #3001
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
4 times, most recently
from
af63eee to
ec77b5c
Compare
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
from
4790bef to
a8346f0
Compare
- Now that copilot is no longer in use in any prod or non-prod environments it is time to remove copilot specific code
* Update documentation that references AWS Copilot
This updates the design of the activity log entry where the patient is added to the session to match how it looks in the prototype.
This updates the design of the table of sessions shown on the patient page to match the latest designs in the prototype.
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
from
a8346f0 to
d0d55fe
Compare
- Ensure we use variable configs to be unique to an environment - For code to reflect deployed configurations - One resource with for-each instead of multiple blocks (DRY) - Remove lifecycle ignore_changes - Ensures regular sync between code and deployed configuration - Any persisted changes will end up being version controlled - Disable PDS/CIS2/SPLUNK for POC environment - Fix typos in copilotmigration and poc environments
When patients join a session half way through a number dates, currently the parents will receive reminders for any session dates prior to when the patient joined. In some cases, this can lead to reminders being sent daily for a number of days. Instead, we can ignore any session dates that exist prior to the patient being added to the session, and in this case, "added to the session" means when the patient received their first consent request. This means that patients won't receive unnecessary reminders in most cases, but I think there is the opportunity for further improvements later with regards to the reminders schedule.
This was only needed temporarily before we supported batches without expiration dates and shouldn't be present in any SystmOne files that we import.
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
4 times, most recently
from
2851c0d to
8d35b96
Compare
- The old service is replaced by identical service with new name
- This used the module and simplifies setup/running/developing in the
future
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
from
1bf5681 to
b537cc4
Compare
- Running terraform apply introduces unnecessary complexity
- Change infrastructure deploy to deploy with most up to date image
- Unless specified otherwise
- Deploy pipeline now can only initiate deploys, not modify any
resources like task definitions
thomasleese
reviewed
Apr 16, 2025
|
|
||
| ```shell | ||
| mise install | ||
| mise exec python -- pip install pre-commit |
There was a problem hiding this comment.
Is this step necessary? In the section above it mentions installing pre-commit via brew or via apt.
There was a problem hiding this comment.
There are 2 pre-commits, IIRC the brew/apt pre-commit is for command line pre-commit <args> which allows you to run hooks/etc manually from command line and the python pre-commit works with the .pre-commit-config.yaml configuration
| language: system | ||
| always_run: true | ||
| pass_filenames: false | ||
| stages: [manual] |
There was a problem hiding this comment.
I think we would still want brakeman to run each time like it does currently in bin/lint.
There was a problem hiding this comment.
It does extend the runtime of the pre-commit hooks to ca. 5 seconds since it acts on all files. Are we sure we want this? We can still choose to run it manually or e.g. inside bin/lint
This splits the jobs in to two separate jobs each, one which runs on a schedule and for each session queues up a separate job which handles the process of sending the communications for a particular session. We're finding that the current job is unable to handle a high number of sessions and runs out of resources. Instead, by queuing a job for each session we can avoid the individual jobs being too resource hungry and gives us more flexibility in terms of running the jobs manually. As part of this change I've also renamed the other jobs for consistency, specifically jobs which are prefixed with `Enqueue` do nothing other than queue up other jobs, and jobs prefixed with `Send` do the task of actually sending out communications.
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
2 times, most recently
from
b7a4cd3 to
4a8fc83
Compare
- Ensure we are formatting properly and consistently
- Users can in principle choose to opt out of automatic hooks by not
installing the tool in the repo
- Simplicity of incluing linting into CI
- For stability we run them initially in parallel to existing linting
scripts
- Scalability and futher development is simple as we can use existing
solutions instead of implementing our own linting setup
- Use existing yarn setup for calling prettier
- Run with --write to replicate linting script behaviour
- Will run in parallel to existing linting script in ci for evaluation
purposes
- Fix formatting before commit - Only applicable to those who opt-in with pre-commit install
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
from
4a8fc83 to
90b419b
Compare
misaka
reviewed
Apr 16, 2025
| ```shell | ||
| apt install curl libyaml-dev libreadline-dev zlib1g-dev \ | ||
| libssl-dev libicu-dev cmake pkg-config uuid-dev flex bison \ | ||
| pre-commit |
There was a problem hiding this comment.
Does pre-commit not list all these other packages as dependencies?
There was a problem hiding this comment.
That is actually just what I had to install to run things when I joined the project (e.g. before pre-commit). I figured I would make a linux section to make future onboarding easier since the mac section did not really work for me
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
2 times, most recently
from
8485be8 to
8216164
Compare
- Act only on .erb files as is the current behaviour of rufo - Exclude the layouts directory in app as was also configured
- Ensure it is only run in manual stage without files passed - This mirrors behaviour of bin/lint - Update workflow to also run manual hooks
- Good standard to apply for formatting - The white-space fixer - Also apply formatting to existing files - Json validity checker - Ensure line endings are `lf` for consistency - Prevent accidental upload of large files - Ensure files end with newline
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
2 times, most recently
from
31bfd64 to
0f7fd4a
Compare
- Ensures we do not make commits that do not follow best practices for terraform
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
from
0f7fd4a to
b5543ec
Compare
- Replace bin/lint with pre-commit implementatino
TheOneFromNorway
force-pushed
the
pre-commit_implementation
branch
from
b5543ec to
09e745b
Compare
|
|
Closing this as we've decided to try |
thomasleese
added a commit
that referenced
this pull request
Apr 24, 2025
This updates our linting set up to use `hk` as the tool runner for our various linters. `hk` is a new tool that is designed to run commands as part of a Git commit, and through that it can be used to run linting tools. It's a proposed alternative to `pre-commit` with some specific goals around running linters in parallel and being highly performant. This is proposed as an alternative to #3001 to avoid the need to add Python to our project, specifically `hk` is provided as a binary and can be included in `.tool-versions` installed by Mise. On the other hand, compared to `pre-commit`, `hk` is a new project and not yet fully supported (there's a warning that things may change in each release on the project website), so we may choose to put this off until a time when `hk` becomes more mature. This comes with similar advantages to `pre-commit`, such as having a standard approach to configuring our linting tools, and for those who wish to, it allows you to integrate the linting with `git commit`. In this PR, I've converting our existing GitHub Actions workflow to a single linting workflow which calls `hk check`. This does mean we lose granularity of individual linters, and if we'd prefer to stick with our existing approach that can be done by using `hk check --linter <linter>`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
installing the tool in the repo
scripts
solutions instead of implementing our own linting setup