Add pre-commit config and a new workflow to run the pre-commit hooks on any PR #503

thesujai · 2024-08-07T17:11:16Z

Linked Issue(s)

Closes #502

Acceptance Criteria fulfillment

Create a .pre-commit-config.yaml file and add the custom listing logic there
Create the GH workflow that will run the pre-commit action
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 lint resulted 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)

CLAassistant · 2024-08-07T17:11:23Z

All committers have signed the CLA.

thesujai · 2024-08-07T17:13:07Z

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:)

jayantbh · 2024-08-07T17:14:50Z

Hey @thesujai, thanks for your contribution!

I'm curious about why flake8 was introduced instead of just using black.

thesujai · 2024-08-07T17:15:20Z

backend/dev-requirements.txt

Should probably bump up the pre-commit here as well

jayantbh · 2024-08-07T17:17:03Z

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. 🤝

thesujai · 2024-08-07T17:26:39Z

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

jayantbh · 2024-08-07T17:50:31Z

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.

thesujai · 2024-08-07T18:05:50Z

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 · 2024-08-16T16:58:35Z

@thesujai there's a conflict, and a failing lint test.

thesujai · 2024-08-16T17:42:01Z

@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!)

jayantbh · 2024-08-21T05:59:50Z

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.

jayantbh · 2024-08-26T12:21:40Z

Hey @thesujai, could you resolve the conflict on this?

thesujai · 2024-08-26T12:49:16Z

@jayantbh Shall work now

thesujai · 2024-08-26T13:08:26Z

Added the related docs as well(Pardon the grammar there 🙃 )

jayantbh · 2024-08-26T15:26:28Z

Thanks! I've triggered the workflows.

adnanhashmi09

@thesujai

why are SVGs also being linted? What is the reasoning behind it.
There are a lot of places in the python code where flake8 is being suppressed. Why?

backend/analytics_server/mhq/api/teams.py

thesujai · 2024-08-26T17:39:43Z

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.

thesujai · 2024-08-26T18:01:51Z

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.

adnanhashmi09 · 2024-08-28T07:28:49Z

@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.

adnanhashmi09

LGTM

samad-yar-khan

Screen.Recording.2024-08-29.at.1.37.04.PM.mov

So, it tells me whats wrong but shouldn't the precommit hook fix these for me ?

CONTRIBUTING.md

samad-yar-khan

Pls update the doc as mentioned above + check if the precommit can lint code on its own.
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

thesujai · 2024-08-29T08:28:03Z

So, it tells me whats wrong but shouldn't the precommit hook fix these for me ?

I can add autoflake for that

jayantbh · 2024-08-29T08:46:43Z

I missed this, but yeah a linter should absolutely try to fix as many issues automatically as it can.

thesujai · 2024-08-29T08:57:16Z

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!

samad-yar-khan · 2024-08-30T08:02:37Z

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 · 2024-08-30T08:58:57Z

@samad-yar-khan I updated the PR

samad-yar-khan · 2024-09-01T09:06:20Z

Was previously using 18, now using 16.19.1

thesujai · 2024-09-01T09:12:37Z

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.

thesujai · 2024-09-01T09:18:23Z

Just like in this workflow, I am installing the lining tools for webserver/ then for cli/ here

adnanhashmi09 · 2024-09-02T11:37:38Z

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

thesujai · 2024-09-02T12:50:32Z

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.

adnanhashmi09 · 2024-09-05T04:48:06Z

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?

adnanhashmi09 · 2024-09-05T04:55:09Z

@thesujai I'd say let's remove the installation of linters from the dev process and make this a part of CONTRIBUTING.md. We'd leave the installation of linters to the contributors and just give them steps how to do that.

Sorry for the slight rework on your end and thanks for your inputs!

…h cli and webserver

thesujai · 2024-09-05T05:54:33Z

Clean now, rebased with main branch as well

adnanhashmi09 · 2024-09-05T06:30:25Z

CONTRIBUTING.md

+
+## Making Commits
+
+1. Pre-commit should be installed as a dev dependency already. If not then Create a virual environment [venv](https://packaging.python.org/en/latest/guides/installing-using-pip-and-virtual-environments/#create-and-use-virtual-environments) or [pyenv](https://github.com/pyenv/pyenv?tab=readme-ov-file#installation) and run the follwoing command from project root dir:


Suggested change

1. Pre-commit should be installed as a dev dependency already. If not then Create a virual environment [venv](https://packaging.python.org/en/latest/guides/installing-using-pip-and-virtual-environments/#create-and-use-virtual-environments) or [pyenv](https://github.com/pyenv/pyenv?tab=readme-ov-file#installation) and run the follwoing command from project root dir:

1. Pre-commit should be installed as a dev dependency already. If not then Create a virual environment [venv](https://packaging.python.org/en/latest/guides/installing-using-pip-and-virtual-environments/#create-and-use-virtual-environments) or [pyenv](https://github.com/pyenv/pyenv?tab=readme-ov-file#installation) and run the following command from project root dir:

adnanhashmi09 · 2024-09-05T06:40:58Z

CONTRIBUTING.md

+    yarn add eslint@^8.40.0 eslint-config-next@13.5.6 eslint-plugin-import@^2.29.0 eslint-plugin-prettier@^5.0.1 eslint-plugin-react@^7.29.4 eslint-plugin-unused-imports@^3.0.0 --dev
+    ```
+    ```
+    cd webserver


Suggested change

cd webserver

cd web-server

Ah good catch!

adnanhashmi09 · 2024-09-05T06:53:54Z

CONTRIBUTING.md

+    cd cli
+    yarn add eslint@^8.40.0 eslint-config-next@13.5.6 eslint-plugin-import@^2.29.0 eslint-plugin-prettier@^5.0.1 eslint-plugin-react@^7.29.4 eslint-plugin-unused-imports@^3.0.0 --dev


No need to install eslint-config-next for cli as it doesn't depend on nextjs.

thesujai · 2024-09-05T07:04:51Z

@adnanhashmi09 should work?

adnanhashmi09

LGTM🚀
It would have been great to lint files inside the container itself, without having the developer worry about linting. But, it's good for now. I will look into a possible solution and if found, will take it up in another PR.

Thanks for the contribution @thesujai. Much appreciated!

outdated

jayantbh · 2024-09-05T10:47:06Z

Amazing. This was a trip. 🚀🚀🚀
Great work @thesujai 👏

thesujai commented Aug 7, 2024

View reviewed changes

backend/dev-requirements.txt Outdated

pre-commit==2.20.0

Copy link

Contributor Author

thesujai Aug 7, 2024

Choose a reason for hiding this comment

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

Should probably bump up the pre-commit here as well

thesujai force-pushed the linting-config branch from fdd7df2 to bcc10e7 Compare August 16, 2024 17:32

thesujai force-pushed the linting-config branch from bcc10e7 to b670cb0 Compare August 26, 2024 12:48

adnanhashmi09 reviewed Aug 26, 2024

View reviewed changes

backend/analytics_server/mhq/api/teams.py Show resolved Hide resolved

adnanhashmi09 previously approved these changes Aug 28, 2024

View reviewed changes

samad-yar-khan reviewed Aug 29, 2024

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

CONTRIBUTING.md Outdated Show resolved Hide resolved

samad-yar-khan previously requested changes Aug 29, 2024

View reviewed changes

thesujai dismissed adnanhashmi09’s stale review via 3b4e6f6 August 29, 2024 08:53

thesujai added 12 commits September 5, 2024 11:19

properly configure lint scripts

8d0b33f

add flake8 as a dev requirements

56f1103

add pre-commit config

c907b09

add pre-commit workflow

e71d6e2

properly configure path of misconfigured lint-fix in cli/

275234a

lint fix all files after adding pre-commit

0aebf56

bump up pre-commit

423a7cf

run node 18 dependencies before 16, so node 16 can do linting for bot…

be47328

…h cli and webserver

lint fix we rebase

2e1ad82

docs update for pre-commit

cb51226

docs update and add autoflake

466398e

add eslint in the contributing guidelines

2672e6e

thesujai force-pushed the linting-config branch from b59f70c to 2672e6e Compare September 5, 2024 05:52

thesujai requested review from adnanhashmi09 and samad-yar-khan September 5, 2024 05:55

adnanhashmi09 requested changes Sep 5, 2024

View reviewed changes

grammatical correction and remove eslint-next for cli

64c96fa

adnanhashmi09 approved these changes Sep 5, 2024

View reviewed changes

adnanhashmi09 merged commit 410ce83 into middlewarehq:main Sep 5, 2024
5 checks passed


		## Making Commits

		1. Pre-commit should be installed as a dev dependency already. If not then Create a virual environment [venv](https://packaging.python.org/en/latest/guides/installing-using-pip-and-virtual-environments/#create-and-use-virtual-environments) or [pyenv](https://github.com/pyenv/pyenv?tab=readme-ov-file#installation) and run the follwoing command from project root dir:

		cd cli
		yarn add eslint@^8.40.0 eslint-config-next@13.5.6 eslint-plugin-import@^2.29.0 eslint-plugin-prettier@^5.0.1 eslint-plugin-react@^7.29.4 eslint-plugin-unused-imports@^3.0.0 --dev

Add pre-commit config and a new workflow to run the pre-commit hooks on any PR #503

Add pre-commit config and a new workflow to run the pre-commit hooks on any PR #503

Conversation

thesujai commented Aug 7, 2024 • edited Loading

Linked Issue(s)

Acceptance Criteria fulfillment

Proposed changes (including videos or screenshots)

Further comments

CLAassistant commented Aug 7, 2024 • edited Loading

thesujai commented Aug 7, 2024 • edited Loading

jayantbh commented Aug 7, 2024

thesujai Aug 7, 2024

Choose a reason for hiding this comment

jayantbh commented Aug 7, 2024

thesujai commented Aug 7, 2024 • edited Loading

jayantbh commented Aug 7, 2024

thesujai commented Aug 7, 2024 • edited Loading

jayantbh commented Aug 16, 2024

thesujai commented Aug 16, 2024

jayantbh commented Aug 21, 2024

jayantbh commented Aug 26, 2024

thesujai commented Aug 26, 2024

thesujai commented Aug 26, 2024 • edited Loading

jayantbh commented Aug 26, 2024

adnanhashmi09 left a comment

Choose a reason for hiding this comment

thesujai commented Aug 26, 2024 • edited Loading

thesujai commented Aug 26, 2024 • edited Loading

adnanhashmi09 commented Aug 28, 2024

adnanhashmi09 left a comment

Choose a reason for hiding this comment

samad-yar-khan left a comment

Choose a reason for hiding this comment

samad-yar-khan left a comment

Choose a reason for hiding this comment

thesujai commented Aug 29, 2024

jayantbh commented Aug 29, 2024

thesujai commented Aug 29, 2024

samad-yar-khan commented Aug 30, 2024

thesujai commented Aug 30, 2024

samad-yar-khan commented Sep 1, 2024

thesujai commented Sep 1, 2024

thesujai commented Sep 1, 2024

adnanhashmi09 commented Sep 2, 2024 • edited Loading

thesujai commented Sep 2, 2024

adnanhashmi09 commented Sep 5, 2024

adnanhashmi09 commented Sep 5, 2024 • edited Loading

thesujai commented Sep 5, 2024

adnanhashmi09 Sep 5, 2024

Choose a reason for hiding this comment

adnanhashmi09 Sep 5, 2024

Choose a reason for hiding this comment

thesujai Sep 5, 2024

Choose a reason for hiding this comment

adnanhashmi09 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

thesujai commented Sep 5, 2024

adnanhashmi09 left a comment

Choose a reason for hiding this comment

jayantbh commented Sep 5, 2024

thesujai commented Aug 7, 2024 •

edited

Loading

CLAassistant commented Aug 7, 2024 •

edited

Loading

thesujai commented Aug 7, 2024 •

edited

Loading

thesujai commented Aug 7, 2024 •

edited

Loading

thesujai commented Aug 7, 2024 •

edited

Loading

thesujai commented Aug 26, 2024 •

edited

Loading

thesujai commented Aug 26, 2024 •

edited

Loading

thesujai commented Aug 26, 2024 •

edited

Loading

adnanhashmi09 commented Sep 2, 2024 •

edited

Loading

adnanhashmi09 commented Sep 5, 2024 •

edited

Loading

adnanhashmi09 Sep 5, 2024 •

edited

Loading