Skip to content

[Feat] Introduce linting in frontend project #187

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

stefanoamorelli
Copy link
Contributor

  • chore: add eslint configuration
  • chore: add lintrc file
  • refactor: fix typecheck issues
  • fix(lint): linting errors
  • ci: add frontend lint check

Solves: #133


We introduce ESlint for the frontend/, fix the existing issues, and add a Github workflow to run the linting and prevent merging of code that does not pass the linting.

@stefanoamorelli
Copy link
Contributor Author

@microsoft-github-policy-service agree

@stefanoamorelli
Copy link
Contributor Author

@matheusmaldaner as promised, here you are! :)

Would appreciate your through review as the PR is medium-sized due to fixing the existing linting error.

@matheusmaldaner
Copy link
Collaborator

Hi @stefanoamorelli, thank you so much for all the changes! Could you review my minor commit? Your linting implementation highlighted some other cases that were failing with the yarn typecheck command, which I have addressed.

Do you know why the Frontend Checks (frontend-lint) are failing in the GitHub CI workflow? I ran the code on my end and there are no errors.

Copy link
Collaborator

@matheusmaldaner matheusmaldaner left a comment

Choose a reason for hiding this comment

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

Good linting changes, do not seem to change anything that could break the code. All tests (poe check) are still passing.

@stefanoamorelli
Copy link
Contributor Author

@matheusmaldaner good stuff! I've reviewed your commit, LGTM left just a couple of comments to confirm.

Also, I've pushed a commit to allow warnings to allow the CI/CD to pass with warnings (given that we don't introduce more warnings).

@matheusmaldaner
Copy link
Collaborator

Requested a review from @husseinmozannar on this so we can merge it before too many changes are made to the system and also so that he can maybe shed light into why the first check is still failing

@stefanoamorelli
Copy link
Contributor Author

Requested a review from @husseinmozannar on this so we can merge it before too many changes are made to the system and also so that he can maybe shed light into why the first check is still failing

Sounds good 👌

@stefanoamorelli
Copy link
Contributor Author

@matheusmaldaner I've rebased the branch and pushed another commit to try fix the failing linting on CI.

Is there anything I can do to help unblock this? Happy to help!

@matheusmaldaner
Copy link
Collaborator

Hi @stefanoamorelli the first check still fails and I am unsure why :/

Could you look into this whenever you got the time or let me know if there is anything I can do on my end to help us solve this issue? Thank you a lot for the contributions and the help with this

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