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

chore: add linting #5

Merged
merged 4 commits into from
Apr 29, 2024
Merged

chore: add linting #5

merged 4 commits into from
Apr 29, 2024

Conversation

dabeeeenster
Copy link
Contributor

@dabeeeenster dabeeeenster commented Nov 27, 2023

  • Adds pre-commit config
  • Runs pre-commit checks as a Github Action

@dabeeeenster dabeeeenster requested review from a team and kyle-ssg November 27, 2023 09:17
@dabeeeenster dabeeeenster requested review from khvn26 and matthewelwell and removed request for kyle-ssg November 27, 2023 09:19
@riceyrice
Copy link
Collaborator

This brings in competing prettier config between the top-level Forge CLI folder and the Jira app folder it contains. The inner folder was intended to be self-contained with the outer folder as a convenience to avoid having to install Forge CLI globally. Notably the outer and inner folder use different versions of Node.js.

@dabeeeenster
Copy link
Contributor Author

Ah - I missed the linting in the app folder. What do you think @matthewelwell ? We're trying to standardise on pre-commit I feel?

@matthewelwell
Copy link
Contributor

Can anyone dumb this down for me so I don't have to understand the structure of the project? What's the issue and what are the options? @dabeeeenster @riceyrice

@dabeeeenster
Copy link
Contributor Author

The app is JS/TS. Should we use native JS/TS provided linters/checkers or use pre-commit which needs python to bootstrap AFAIUI

@riceyrice
Copy link
Collaborator

Can anyone dumb this down for me so I don't have to understand the structure of the project? What's the issue and what are the options? @dabeeeenster @riceyrice

@matthewelwell the root of the repo contains/installs a Node 20 app (the Forge CLI). The flagsmith-jira-app folder contains/builds a Node 16 app (the Jira app). Both create a node_modules directory with different dependencies.

It's OK to install linting/formatting tools in the root if you need to make this work with pre-commit - or maybe pre-commit can be made to work with the linting/formatting tools already installed in the flagsmith-jira-app folder. If you do put the tools/config in the root folder, the existing tools/config in the app folder should be removed to avoid conflicts.

@matthewelwell
Copy link
Contributor

If you do put the tools/config in the root folder, the existing tools/config in the app folder should be removed to avoid conflicts.

This makes the most sense to me.

@dabeeeenster
Copy link
Contributor Author

OK that should be in line with what we want now

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this have been removed as per discussion in the main PR thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its needed in both locations for both to work AFAICT

@matthewelwell
Copy link
Contributor

@dabeeeenster not sure what the status of this one is but it seems ok to me, shall we merge?

@dabeeeenster dabeeeenster merged commit a3dd3c1 into main Apr 29, 2024
1 check passed
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.

3 participants