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

M2-4835: Linting rules for mindlogger-admin #1472

Merged
merged 37 commits into from
Feb 21, 2024

Conversation

sultanofcardio
Copy link
Contributor

@sultanofcardio sultanofcardio commented Feb 6, 2024

resolves: M2-4835

This PR creates a GitHub Action to run linting tasks on pull requests submitted to the repository. As part of the ticket, I took the opportunity to update the ESlint rules on the project. These updates have raised a number of linting issues, but won't block the merging of pull requests. While I've addressed some of the smaller items (and any error ones), I expect that these will be addressed in the normal course of feature development so that we can eventually require linting checks to pass in order to merge PRs.

Here's a summary of what changed:

Conflicting ESLint configurations

There are currently two ESLint configurations in the project:

  • An eslintConfig property in the package.json file
  • A .eslintrc file

ESLint documentation says that only one of these is read, and the .eslintrc takes precedence. Thus, I've removed the ignored configuration since it wasn't being used.

Prettier Code Formatting

Prettier has been configured in the project as the code formatter of choice, but it is currently conflicting with a number of code formatting ESLint rules. The prettier ESLint plugin disables these rules if it is placed as an inherited configuration at the end of the extends array, so I have made this change. The prettier ESLint plugin also enables Prettier as an ESLint rule, so that code formatting issues become errors. This should be okay, since Prettier fixes them for you.

In addition, I've disabled/removed some other code formatting related rules that aren't explicitly handled by the Prettier plugin. These include:

  • padding-line-between-statements
  • no-extra-semi
  • jsx-quotes - I've moved this one to the Prettier config with jsxSingleQuote

Opinionated changes

There are a number of other changes I've made that I think will allow for improved code quality. These are:

  • Turning on the react-hooks/exhaustive-deps rule
  • Adding the unused-imports ESLint plugin
  • Turning on the @typescript-eslint/no-non-null-assertion rule

@sultanofcardio sultanofcardio self-assigned this Feb 6, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1472.dpmzgfnfrj2mv.amplifyapp.com

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.prettierrc Outdated Show resolved Hide resolved
.prettierrc Outdated Show resolved Hide resolved
.prettierrc Outdated Show resolved Hide resolved
.prettierrc Show resolved Hide resolved
@IhorNesterenko99
Copy link
Contributor

I've noticed that the unit tests are failing after the changes made in the pull request. Could you please take a look and correct any issues with the unit tests?

Additionally, there are some conflicts that need to be resolved before this pull request can be merged. Could you please review and resolve the conflicts accordingly?

@sultanofcardio
Copy link
Contributor Author

I've noticed that the unit tests are failing after the changes made in the pull request. Could you please take a look and correct any issues with the unit tests?

Additionally, there are some conflicts that need to be resolved before this pull request can be merged. Could you please review and resolve the conflicts accordingly?

@IhorNesterenko99 Yes, I will address these. I initially believed that the tests were failing because of this commit, but after rebasing again I realised they are still failing.

@sultanofcardio sultanofcardio force-pushed the feature/M2-4835-linting branch 2 times, most recently from d96535b to af655ac Compare February 12, 2024 13:43
@IhorNesterenko99
Copy link
Contributor

IhorNesterenko99 commented Feb 12, 2024

I wanted to bring to your attention that our Jenkins continuous integration is currently failing. Here's a screenshot for reference:
image
It might be a good idea to reach out to our DevOps lead, Raman Siauko, to investigate what might have caused this issue.

Copy link
Contributor

@alex-i-e alex-i-e left a comment

Choose a reason for hiding this comment

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

Perfect! You've added meaningful and valuable lint rules for ongoing on project, some of them will point on places that should be fixed in future. I appreciate your input and contribution to improving quality!

The linting job creates check runs (ESLint, Prettier) which detail their respective outcomes, but the job itself does not fail if there are linting errors. This changes that
It seems that some of the test files depend on importing mocks and other things in a specific order, so I'll revert this for now
Prettier apparently recommends no integrating prettier with linters at all. It also seems that this integration no longer works well (or I've misconfigured them) because prettier and ESLint are making conflicting changes. Therefore, I've opted to run them separately
I also included all the files from `.lintstagedrc.json`
This action seems to be reporting Prettier issues where there are none, so I'm trying something else
As it turns out I was accidentally using a global prettier binary (v2.7.1) instead of the one from the project, which has led to a linting headache for the last few hours
@sultanofcardio sultanofcardio merged commit 57051d6 into develop Feb 21, 2024
5 checks passed
@sultanofcardio sultanofcardio deleted the feature/M2-4835-linting branch February 21, 2024 15:39
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.

4 participants