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-web-refactor #370

Merged
merged 12 commits into from
Feb 22, 2024

Conversation

sultanofcardio
Copy link
Contributor

@sultanofcardio sultanofcardio commented Feb 8, 2024

resolves: M2-4835

Objective

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 to be more in line with those on mindlogger-admin. 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.

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.

Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

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

I'm very happy with these changes, thanks @sultanofcardio. I just left one minor comment about max line length; hoping we can all align on having consistency here (at least between the FE apps, if not the BE).

.prettierrc.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mbanting mbanting left a comment

Choose a reason for hiding this comment

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

My review only focused on .eslintrc.js and .prettierrc.js files. Briefly looking at the rest of the changes they're a result of the changes to these aforementioned files. Thanks for putting this together and incorporating these updated rules.

Is it possible to hold off on merging until Wed Nov 21? We've been requested to code freeze the dev2.5 branch to accomodate it's release stabilization. I realize this delay will significantly increase the chance of code conflicts. Maybe this is the first PR that gets merged as soon as the code freeze is lifted.

@sultanofcardio
Copy link
Contributor Author

Is it possible to hold off on merging until Wed Nov 21

Sure thing @mbanting. I held off ChildMindInstitute/mindlogger-admin#1472 for the same reason 👍

@mbanting
Copy link
Contributor

mbanting commented Feb 14, 2024

Is it possible to hold off on merging until Wed Nov 21

Sure thing @mbanting. I held off ChildMindInstitute/mindlogger-admin#1472 for the same reason 👍

@sultanofcardio actually feel free to merge that one. We've only been ask to code freeze the dev2.5 branch for this repo, mindlogger-web-refactor. Thk you!

Copy link
Contributor

@moiskillnadne moiskillnadne left a comment

Choose a reason for hiding this comment

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

In my humble opinion, the semicolon in JS code makes it more overloaded for the eyes. Probably it is just my habit.

But to make the codebase consistent between repositories we should do it. It looks good to me.

Resolve conflicts please and merge it.

@moiskillnadne moiskillnadne added Done Feature or bug done Enhancement labels Feb 21, 2024
@sultanofcardio sultanofcardio merged commit 0abce1e into dev2.5 Feb 22, 2024
5 checks passed
@sultanofcardio sultanofcardio deleted the feature/M2-4835-linting branch February 22, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Done Feature or bug done Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants