middleware
middleware copied to clipboard
middlewarehq
Add pre-commit config and a new workflow to run the pre-commit hooks on any PR
Linked Issue(s)
Closes #502
Acceptance Criteria fulfillment
- [x] Create a .pre-commit-config.yaml file and add the custom listing logic there
- [x] Create the GH workflow that will run the pre-commit action
- [x] Format the entire codebase with pre-commit hooks
Proposed changes (including videos or screenshots)
- Change the scripts command in package.json of both node sub dir(cli/ and web-server/). As eslint should be ran with a proper path. Earlier running
yarn lintresulted in nothing because paths were not configured - Introduced pre-commit to Run before committing anything. As of now, I have setup for only linitng and formatting files before committing. I can integrate more pre-commit hooks if you want one. But these are the ones I think are necessary.
- Added a new GH workflow to run the pre-commit hooks.(Just saying it would have been a little simpler if both web-server/ and cli/ used the same version of node)(So I setup two node versions in the workflow)
Further comments
- I tested this with a test PR. Though you see the hooks failed there, but I created that PR to see if the workflow setup runs correctly or not. It does work.
- Now After you review this PR, I will also push the formatted files(Not pushing now as reviewing would be gnarly for the maintainers)
Just so you know we dont need a separate black workflow with this one.(So can be removed)
Feel free to close it if the feature is not desired for now:)
Hey @thesujai, thanks for your contribution!
I'm curious about why flake8 was introduced instead of just using black.
One of the checks failed. Interesting. If you could look into sorting it out (our team will help), we'd love to accept this PR. 🤝
Hi @jayantbh thanks for a quick review Flake8 does so much more than just lining the code(It checks for unused imports, also potential bugs!) Like for eg the below code will be given a green flag by black:
import os
print("Hello")
But flake8 will throw an F401(Imported but not used) as the os module is imported but never used. For now black ignores such best practices and focuses on PEP8 guidelines(Which don't include a F401 type thing!). So most error thrown by flake8 should be manually fixed(As no library I know of does things like this automatically)
One of the checks failed
Yes, it is supposed to fail, I just introduced it with this PR. It will pass once I push all the formatted files(formatted by the pre-commit hooks). I am not pushing now as reviewing would be really irritating with so many lint-fixed files
Sounds good. I'd like @samad-yar-khan and @adnanhashmi09 to confirm if flake8 plays well with our dev setup, so we can replace black with it.
I am just bad with explaining my thoughts
More like having flake8 along with black(as black is just the formatter, flake8 is just a checker). But both of these we will have as pre-commit hooks(with the pr introduces).
Just the thing to remove will be the black github action, as black will be ran as the last hook of pre-commit with the new pre-commit workflow.
@jayantbh It should work now. I rebased it with main.
Any changes in the upstream will again make the workflow to fail if this gets merged, so we have to make sure this is up to date with main before it is merged(it is now!)
Hey @thesujai, this PR is currently on pause because we're waiting for some other PRs to get merged to avoid rework on them. We'll take it up again in 1-2 days. Apologies for the slow movement on this.
why are SVGs also being linted? What is the reasoning behind it.
Most probably they didn't have the extra blank line at the eof.
why are SVGs also being linted? What is the reasoning behind it.
Most probably they didn't had the extra blank line at the eof.
There are a lot of places in the python code where flake8 is being suppressed. Why?
Mostly I am doing to ignore the warning when we use ! = instead of is not for comparing objects like None.
I suppressed that rule for now, but we can have a follow up to remove such object comparisons.
@thesujai Got it. Thanks for the clarifications. We should have a followup PR to have these comments removed. But for now it looks good to me.
So, it tells me whats wrong but shouldn't the precommit hook fix these for me ?
I can add autoflake for that
I missed this, but yeah a linter should absolutely try to fix as many issues automatically as it can.
Preferably, if someone is using /.dev.sh for dev, they are looking to change the code. See if we can get them to install the precommit at the top rather than potential misses and a lint commit at the end when they discover the precommit hook
I don't understand what you trying to say here. Let me know if the latest commit solves your concern!
Preferably, if someone is using /.dev.sh for dev, they are looking to change the code. See if we can get them to install the precommit at the top rather than potential misses and a lint commit at the end when they discover the precommit hook
I don't understand what you trying to say here. Let me know if the latest commit solves your concern!
Hey @thesujai , that last commit solves a part of the problem of automatic linting with pre-commit hook. This still does not solve the effort of manually installing python dependancies just to run the precommit hook.
Currently our dev setup only requires the user to use the ./dev.sh to develop using docker. All things needed by the dev setup should be a part of that script. This means, if I use dev.sh, it should automatically check if I need to install precommit hooks or any dependancies, install them at system level or in a virtual env and activate venv (in case of python) and let the dev build and commit without having to manually go and install these just to commit.
@thesujai I see the updated dev.sh, nice work on that. I don't have to install python packages now 🚀 . But, its not working for me. Also, one of the linting workflows is failing.
https://github.com/user-attachments/assets/ebfcc4bd-330c-4484-963c-19c1ea1b2fb5
From here it looks like your cli/ had node modules installed, but not your webserver/.
We can configure dev.sh to also install the node_moduels(locally, not in a docker container), but this would mess up the current workflow where node_modules are getting installed into docker containers.
I suggest doing this but only adding eslint(and other linting tools) to the dev.sh workflow that will install node_modules(only with required tools) locally. But this would be extra work to keep maintaining the tools versioning in package.json as well as the dev.sh script.
Let me know if you have other ideas.
How I like to view things is that development is one thing and contributing is another.
I don't like where this is headed, installing node_modules and python dependencies on both the container and the host machine. This kind of defeats the purpose of having a development setup with containers in the first place. Can't we just use docker volumes to map the node_modules and python dependencies to the host's filesystem? @thesujai @samad-yar-khan
Can't we just use docker volumes to map the node_modules and python dependencies to the host's filesystem?
Possible, but there are some tradeoffs:
- It might compromise the container’s isolation by exposing dependencies to the host. The major advantage of Docker is keeping dependencies isolated within the container.
- Docker volumes can cause compatibility problems across different OSs, especially with node_modules. The docker base image isn't the same as host OS.
Possible, but there are some tradeoffs:
- It might compromise the container’s isolation by exposing dependencies to the host. The major advantage of Docker is keeping dependencies isolated within the container.
- Docker volumes can cause compatibility problems across different OSs, especially with node_modules. The docker base image isn't the same as host OS.
Yeah makes sense. To run the linting process we would have to install node_moduels and python dependencies anyhow. Maybe we could remove it from the dev process and add to the contributing docs. This is making things a little messy in my opinion.
@jayantbh Thoughts?