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

Add ESLINT and Prettier with autosave #212

Merged

Conversation

vaibssingh
Copy link
Contributor

@vaibssingh vaibssingh commented Jul 25, 2023

Closes #193

Changelog:

  • Install the most relevant ESLINT and Prettier modules for the project
  • Create a set of scripts in the package.json to mass lint and prettify
  • Ensure ESLINT and Prettier warnings are visible in IDE
  • Auto linting and pretty on save (works in VS Code)
  • Bonus points 🏆 - create a GitHub Action to validate all files are linted and pretty

Notes:

  1. Some of the rules in eslintrc have been disabled/modified to avoid conflicting rules between eslint & prettier and to follow the existing standards of the repo.
  2. The quotes rule has been disabled in some of the files due to it creating problems with existing code and producing more eslint issues.
  3. I observed that while auto lint works on explicitly saving file using Cmd+S, it doesn't work with autosave enabled in VSC. So you have to do Cmd+S manually for auto lint to work. It is the expected behaviour in VSC as discussed in the thread below so it should not be considered an issue:
    Don't run codeActionsOnSave on focus changes microsoft/vscode#121155

@vaibssingh vaibssingh marked this pull request as draft July 25, 2023 17:55
@vaibssingh vaibssingh changed the title Feat/193 add eslint prettier autosave add eslint and prettier with autosave Jul 30, 2023
@vaibssingh vaibssingh marked this pull request as ready for review July 31, 2023 00:33
@vaibssingh
Copy link
Contributor Author

Hey @JamieSlome! Sorry about the delay with this PR. Didn't find time from daily work to close this sooner.
Have added all the points as mentioned in your task list. The GitHub action to verify the linting workflow works as well. Let me know if there is anything else that's needed to be done on this 😃

@JamieSlome
Copy link
Member

@TheJuanAndOnly99 - can we make the linting check a requirement for PRs prior to merging in the repository settings?

@maoo
Copy link
Member

maoo commented Jul 31, 2023

This is how the main branch protection looks like now. Is that ok?

Screenshot 2023-07-31 at 19 28 34

@JamieSlome
Copy link
Member

@vaibssingh, my last question is, how will the results be reported to the developer? In the GitHub Actions console?

@JamieSlome
Copy link
Member

@maoo - sorry to be a pain, but can we disable the check requirement for the time being? It seems to be preventing the merging of all other PRs, and I still need to review the contents of this PR 🤗

@vaibssingh
Copy link
Contributor Author

@vaibssingh, my last question is, how will the results be reported to the developer? In the GitHub Actions console?

@JamieSlome Yep. It would look something like this
https://github.com/finos/git-proxy/actions/runs/5709151849/job/15467588676

@TheJuanAndOnly99
Copy link
Member

@JamieSlome I've removed the linting check for now. Let me know when you're ready and I can enable it again.

@JamieSlome
Copy link
Member

Thank you, @TheJuanAndOnly99 😄

@JamieSlome JamieSlome changed the title add eslint and prettier with autosave Add ESLINT and Prettier with autosave Aug 3, 2023
@JamieSlome
Copy link
Member

@vaibssingh - my bad, are you able to resolve the merge conflicts? Deleting the package-lock.json and npm i should do it 👍

@JamieSlome
Copy link
Member

@vaibssingh - looks like there is still a merge conflict for some reason. Once that is addressed, we can merge! 🎉

@vaibssingh
Copy link
Contributor Author

@JamieSlome Fixed the conflict but now it is failing the CVE test 😭

@JamieSlome
Copy link
Member

@vaibssingh - do you want to give the latest merge conflict a go and see if that resolves the CVE breakage? 👍 Thank you for your patience! 🍰

@vaibssingh
Copy link
Contributor Author

Hey @JamieSlome , I have tried multiple times for this to work but I am not able to get it resolved. The material-ui requests @types/react@17.0.63 but during npm install using node v16.19 it installs @types/react@18.2.19. Since we have npm ci instead of npm i for this CVE workflow, on encountering this conflict it stops the pipeline which causes it to fail. Do you have any idea how we can fix it?

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰

@JamieSlome JamieSlome merged commit 5f18c5e into finos:main Aug 11, 2023
5 checks passed
coopernetes pushed a commit to coopernetes/git-proxy that referenced this pull request Oct 13, 2023
…tier-autosave

Add ESLINT and Prettier with autosave
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.

Implement best ESLINT and Prettier combo with auto save 💪
4 participants