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-5381: Add initial set of linting rules for mobile repository #632

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

sultanofcardio
Copy link
Contributor

📝 Description

🔗 Jira Ticket M2-5381

Note

This is a follow-up to #617

This PR creates an initial set of linting rules with a focus on enhancing code quality and maintainability without imposing strict stylistic guidelines. Each added rule should fall in one of the following categories:

  • Potential Bugs Detection: Rules that help identify common coding mistakes that could lead to runtime errors or unexpected behaviour.
  • Performance Issues: Rules focused on optimising performance, such as detecting unnecessary re-renders in React components or inefficient database queries.
  • Stale Code Identification: Rules to detect unused variables, functions, components, or other elements that can clutter the codebase and reduce maintainability.
  • Security Vulnerabilities: Basic security linting rules that flag potentially unsafe coding practices, such as improper handling of user input.

This PR isn't meant to be exhaustive, and so there may be rules I have overlooked that could be added now (or later on as the need arises).

✏️ Notes

Please be generous, but focused with your feedback. I'm especially interested in feedback of this nature:

  • Point out rules that you don't think fall into one of the above categories, and you think should be removed
  • Highlight out any other rules you think should be removed, and share your reasoning
  • Suggest any additional rule that you think should be added now. If it doesn't fall into one of the categories I mentioned, then please share your reasoning

P.S. I'm mindful that this PR is potentially blocked by the work being done on the rn-update-0.73.4 branch. Please let me know if there are additional considerations I should make with that update in mind.

@sultanofcardio sultanofcardio self-assigned this Feb 22, 2024
@sultanofcardio sultanofcardio marked this pull request as ready for review February 22, 2024 21:00
Base automatically changed from feature/M2-4835-linting to dev February 22, 2024 21:35
@sultanofcardio sultanofcardio force-pushed the feature/M2-5381-linting-refix branch from 586321f to d64329f Compare February 22, 2024 21:37
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.

Did a scan the the rules and it's a good starting set that focuses on detecting potential bugs, performance issues, stale code, and security vulnerabilities. It looks like some of the more subjective or stylistic rules from the previous PR were removed as intended.

As mentioned in another PR and as you stated in the PR description, there will be conflicts with the rn-update-0.73.4 branch although I had a look and it should be relatively straightforward to resolve (at least the .eslintrc.js file).

@BamMironov suggested waiting for that to be merged to dev first before any further linting changes like this. Let's hold off merging this (but still aim to have it in before sprint end) until we have a better idea of where that's at. Alternatively you can already resolve the potential conflicts with that branch here.

By waiting we also give @BamMironov a chance to have a look at this too.

Copy link
Contributor

@BamMironov BamMironov left a comment

Choose a reason for hiding this comment

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

Looks good to me!
But yes, let's hold it off till the RN update is merged.

@sultanofcardio sultanofcardio force-pushed the feature/M2-5381-linting-refix branch from d64329f to f672316 Compare April 26, 2024 17:09
@sultanofcardio
Copy link
Contributor Author

The changes from rn-update-0.73.4 have been merged into dev, so I rebased and resolved merge conflicts. I'll defer merging this until EOD Monday to give folks a chance to re-review

@BamMironov BamMironov merged commit 7db12a9 into dev Apr 29, 2024
4 checks passed
@BamMironov BamMironov deleted the feature/M2-5381-linting-refix branch April 29, 2024 08:11
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