M2-4835: Linting rules for mindlogger-app

[sultanofcardio]

📝 Description

🔗 Jira Ticket M2-4835

This PR creates a GitHub Action to run linting tasks on pull requests submitted to the repository. By default, any issues raised won't block the merging of PRs - they are currently only for developer feedback.

✏️ Notes

No linting or code formatting rules have been modified

[BamMironov]

Please let me know what was the goal of the PR.
If the goal was to fix ESLint errors that happen on Github actions then it would be a good idea to fix the Github actions?
As of today, developers are comfortable and used to the formatting styles that are set up in the project. Linting errors also work as expected and ESLint highlights them in the text editor accordingly. If you have issues with your text editor, please get in touch with me and I'll help you set it up.
If the goal was to match the code style of the completely independent projects (web, admin) please let me know why it is needed and what advantages it may bring about.
I think it would have made more sense to do it in the early stage of the project and not when the project is set up and already in Production.

In general, the total Diff between the changes is huge. In light of the test coverage (there are still too less of them) I would not merge it and consider it risky.

[sultanofcardio]

@BamMironov Thanks for providing feedback. Sorry that the PR description doesn't make this clear. I'm happy to provide more clarity regarding my intent.

Please let me know what was the goal of the PR.

The primary goal of the PR is to define and enable a GitHub Actions job that will eventually block the merging of PRs that don't align with the formatting and linting rules in the project. As it stands today, I can easily bypass the lint-staged pre-commit task, so this will add more confidence for reviewers.

A secondary goal is to propose a core set of linting rules that may be applicable across the three front end projects:

• M2-4835: Linting rules for mindlogger-admin mindlogger-admin#1472
• M2-4835: Linting rules for mindlogger-web-refactor mindlogger-web-refactor#370
• M2-4835: Linting rules for mindlogger-app #617

If the goal was to fix ESLint errors that happen on Github actions then it would be a good idea to fix the Github actions?

This isn't an immediate goal. Fixing all of these issues would lead to a diff that is too large, so I've turned them all into warnings so they may be addressed throughout the course of normal feature development.

As of today, developers are comfortable and used to the formatting styles that are set up in the project. Linting errors also work as expected and ESLint highlights them in the text editor accordingly. If you have issues with your text editor, please get in touch with me and I'll help you set it up.
If the goal was to match the code style of the completely independent projects (web, admin) please let me know why it is needed and what advantages it may bring about.

The change is desirable because it helps to lower the cognitive load of switching between repos for those developers who are working on multiple repos. However, it isn't my goal to cause disruption and I would like to avoid it as much as possible. I welcome feedback on the rule changes proposed. This change isn't being made in isolation and I expect it may take some discussion to come to an agreement across the repos.

In general, the total Diff between the changes is huge. In light of the test coverage (there are still too less of them) I would not merge it and consider it risky.

All of the changes made to the src directory are stylistic and were made using eslint --fix and prettier --write. I thought it made sense to at least address the auto-fixable stuff. I agree that such a large diff can be difficult to evaluate and I'm happy to defer those changes to also be made over the course of normal feature development

[farmerpaul]

Thanks for taking this on @sultanofcardio! The new rules look great to me, and I appreciate being aware of the many warnings these rules reveal. It'll be great to clear those up over time. I had one suggestion around maximum line length that I hope we can get team alignment on.

I also identified an issue that is causing an issue with app, but it's a quick fix. Preapproving, pending that change!

[BamMironov]

The primary goal of the PR is to define and enable a GitHub Actions job that will eventually block the merging of PRs that don't align with the formatting and linting rules in the project. As it stands today, I can easily bypass the lint-staged pre-commit task, so this will add more confidence for reviewers.

Absolutely! I agree that it should be the primary goal of the PR. Indeed, there is a way to bypass the lint-staged task and it would be beneficial to enforce the same tasks in Github actions.

The changes made to the linting rules are irrelevant to the main goal, in my opinion, and should be discussed separately. Speaking of which, I still don't see any benefits of having the same linting rules.

it helps to lower the cognitive load of switching between repos for those developers who are working on multiple repos

How many people are working with the multiple repos (especially on Mobile)? What exactly is the cognitive load of switching between repos that you mentioned? We have people who can switch from project to project. None of them experienced the cognitive load. Is it the IDE's responsibility to automatically format the code upon file saving?
The projects here are very independent from each other. For instance, web applications are different from mobile ones. Also, they are different from the design of architecture point of view. If it was a problem then we would have complaints from developers or even bugs. I have not seen any of them. What problem are we trying to solve right now?

TLDR: Let's focus on the main goal of the PR which is executing lint-staged tasks in the GitHub Actions.

[mbanting]

Thanks @BamMironov for your thoughtful feedback. We're all aligned on the importance of updating GitHub Actions to automate our linting tasks, ensuring code quality and consistency across our contributions.

I understand the reservations about simultaneously updating the linting rules. Decoupling these updates is an option, focusing initially on the GitHub Actions integration. However, I'd like to emphasize the broader context and strategic advantages that these updates bring in an ongoing effort to improve MindLogger platform development.

As you mentioned, the projects are indeed modular and independent system-wise, however, they are connected from a product development standpoint. As teams expand and focus shifts more towards feature development, finding common ground in our development practices across the repositories becomes increasingly beneficial. This unified approach is not just about simplifying the developer's workflow but also about aligning development practices with long-term product and engineering objectives. It's in the best interest to do what we can to maximize flexibility in allocating devs to different areas as these priorities shift over time.

The Meta pod developers are working across the different repositories. The decision to use a common language (JavaScript/TypeScript) and framework (React/React Native) across projects makes this possible. By aligning the linting rules on this common language and framework, we further this goal, facilitating easier transitions between projects, reducing the time needed to switch between differing standards, and ultimately improving productivity and quality.

The benefits extend beyond easing cognitive load and streamlining the PR review process. This alignment is one small but still nice step that makes achieving other broader goals slightly easier, including:

• Achieving feature parity across front-ends.
• Establishing a common design system.
• Facilitating code sharing and reuse.

We are committed to making these transitions as smooth as possible and open to incorporating these changes with smaller disruption. If you're still not comfortable with this PR, perhaps we can move forward with a more focused subset of rules and gradually expand from there. Thoughts?

[BamMironov]

@mbanting Thank you for your detailed response and for outlining the strategic advantages you see in updating GitHub Actions and aligning linting rules across our repositories. While I appreciate the effort to streamline development practices and improve productivity, I have some reservations about the proposed approach.

Firstly, I believe that decoupling the updates, as suggested, might indeed be a more prudent approach. Focusing solely on the GitHub Actions integration initially could help mitigate potential risks and allow for a smoother transition.

Additionally, while I acknowledge the benefits you've outlined, I believe we should proceed with caution and carefully assess the impact of these updates on each project individually. Perhaps we can explore a more tailored approach that allows for flexibility while still achieving the desired level of consistency and quality across our contributions. However, I don't believe having the same linting rules will facilitate the speed of development or code quality, or make transitions between projects easier. Once again, projects are very different in many key aspects.

• Achieving feature parity across front-ends.
• Establishing a common design system.
• Facilitating code sharing and reuse.

I couldn't agree more with the importance of them! These are indeed key points that deserve our attention when discussing a unified approach. Moreover, I believe focusing on these aspects will not only speed up development and increase velocity but also make testing much easier, whether it is manual testing or auto-tests.

Finally, the integration of IDEs plays a pivotal role in ensuring smooth transitions for developers between backend, front-end, and mobile projects. Features like "Auto format on save" alleviate the burden of code styling, allowing developers to concentrate solely on the business logic and value-added changes. While I understand the frustration of encountering linting errors, particularly when they seem trivial (for instance, you wrap function arguments in brackets and the editor is screaming at you saying that you should not use them here), the automatic correction upon saving significantly mitigates this inconvenience. However, I believe it's essential to weigh the benefits of uniform linting rules against the potential drawbacks. While consistency is valuable, enforcing uniformity solely for the sake of consistency may not necessarily enhance the project's overall value. Instead, our focus should remain on fostering efficiency and productivity, leveraging tools like IDEs to streamline development processes while allowing for flexibility where necessary.

Thank you for considering my perspective! I'm open to further discussion on how we can best move forward.

[mbanting]

@BamMironov while we may not be on the same page with the entire pull request, it's great that we agree that the update to GitHub Actions is something to move forward with.

We'll update this PR and remove the rule updates leaving this just for the GitHub Actions integration.

We will create a separate ticket and PR to move forward with a smaller subset of linting rule changes. It will focus less on subjective code styles and rules for consistency, and instead include the rules that were intended to highlight code smells (ie. potential bugs or bad performance) and result in cleaner code. I think we can all agree that this is needed and will make it easier for developers to contribute high quality code.

[BamMironov]

@mbanting Thank you! One more thing I think is worth mentioning: I would like to propose to start making those changes only after the RN update (which can be found on rn-update-0.73.4 branch) is tested and merged to dev as it also has changes that may conflict with the new rules.

[sultanofcardio]

@BamMironov I've reduced the set of changes to just include the GitHub Actions workflow that performs linting (in addition to the test suite)

[mbanting]

Looks good. My only request is to update the PR description to account for the fact the rule / prettier updates are no longer a part of this PR based on earlier PR feedback.

I think we've reached consensus, and Artem is out sick so he's unable to approve now