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

Enforce JS formatting standards to improve readability and encourage collaboration #2096

Closed
robyngit opened this issue Feb 8, 2023 · 5 comments · Fixed by #2412
Closed
Assignees

Comments

@robyngit
Copy link
Member

robyngit commented Feb 8, 2023

Describe the feature you'd like
Select a code formatting standard, and enforce it using a shared JS formatter tool / linter. Reformat all JS files in MetacatUI to align with this standard. Consider also implementing a pre-commit hook to prevent unstyled code from being committed.

Is your feature request related to a problem? Please describe.
Code in MetacatUI uses a combination of tabs and spaces for indenting, and has many other stylistic inconsistencies. The result is that the code tends to be difficult to read, understand, and update. I believe these challenges are a barrier to onboarding new developers and discourage contributions from external developers.

Additional context
Given that this will cause major merge conflicts for users who have extended MetacatUI, and that any code that doesn't follow the new standard will not be valid, this should be considered a breaking change. This was discussed as part of the the 3.0.0 release.

@robyngit
Copy link
Member Author

robyngit commented Feb 8, 2023

There are plenty of formatters, linters, and standards to choose from. I haven't done a deep dive into what's available, but standardJS looks really nice. Here are some highlights:

  • No config required: "At the end of the day you have to 'just pick something', and that's the whole philosophy of standard -- its a bunch of sensible 'just pick something' opinions."
  • Supports extensions for Atom, VSCode, Vim, Emacs, Brackets & Webstorm
  • Can use in pre-commit hooks
  • Has an automatic formatter
  • Widely used (27.9K stars on GitHub)

@robyngit robyngit added this to the MetacatUI 3.0.0 milestone Feb 9, 2023
@robyngit
Copy link
Member Author

We should also consider prettier. From @ianguerin:

I usually opt for prettier because it has very little configuration, it makes a lot of its own style decisions rather than letting me make them

@robyngit robyngit modified the milestones: MetacatUI 3.0.0, 2.29.0 Apr 18, 2024
@robyngit robyngit self-assigned this Apr 18, 2024
@ianguerin
Copy link
Collaborator

ianguerin commented Apr 19, 2024

Prettier also plays nicely with eslint with projects like eslint-config-prettier that ensure there are not conflicting rules with having both configured.

Eslint seems to be more extensible, it lets you define your own rules, so you can enforce things like property name ordering, or import ordering, the possibilities are great.
I was playing around with eslint's playground to try to restrict a specific property from Backbone.View declarations, and it even provides a shareable link!

@robyngit
Copy link
Member Author

I've explored ways to minimize merge conflicts for projects that have extended MetacatUI. Here's the approach I tested:

Setup:

  • Created a branch from develop simulating an extended MetacatUI project with various modifications (adding/removing methods, altering CSS, etc.).
  • Created another branch from develop and configured it with the prettier setup, applying formatting to all files.

Test 1: Merging both branches

  • Merged the prettier-formatted branch into the modified branch.
  • Result: This resulted in confusing merge conflicts, making it challenging to discern changes from formatting differences.

Test 2: Applying prettier before merging

  • On the modified branch, cherry-picked the prettier configuration commit.
  • Ran prettier on all files and committed these changes.
  • Merged the prettier-formatted branch into this now formatted modified branch.
  • Result: This approach was much more effective. The merge conflicts showed actual changes rather than formatting differences.

This indicates that proactive formatting could mitigate the impact of such changes on extended versions of MetacatUI. Since merge conflicts are somewhat inevitable when integrating updates, the additional formatting conflicts should be manageable. We could guide users on pre-formatting their code before merging the latest updates. Additionally, we could consider releasing linting changes separately from other updates in order make it easier for users to format their code and resolve conflicts independently of features/bug fixes. Otherwise, we could provide the exact commit hash for the formatting config, enabling users to cherry-pick this commit and apply prettier themselves.

@vchendrix @helbashandy @yvanlebras - Given your experiences with extending MetacatUI, I’d love to hear your feedback on this approach!

@robyngit robyngit modified the milestones: 2.29.0, 2.30.0 May 6, 2024
robyngit added a commit that referenced this issue May 13, 2024
- Initial prettier setup; do not format yet
- Fix syntax errors in files that would prevent prettier from formatting them

Issue #2096
robyngit added a commit that referenced this issue May 13, 2024
- Initial prettier formatting run

Issue #2096
@robyngit robyngit linked a pull request May 13, 2024 that will close this issue
robyngit added a commit that referenced this issue May 13, 2024
- Initial prettier setup; do not format yet
- Fix syntax errors in files that would prevent prettier from formatting them

Issue #2096
robyngit added a commit that referenced this issue May 13, 2024
- Initial prettier formatting run

Issue #2096
robyngit added a commit that referenced this issue May 20, 2024
globally defined packages are specified in eslint config

Issue #2096
robyngit added a commit that referenced this issue May 20, 2024
globally defined packages are specified in eslint config

Issue #2096
robyngit added a commit that referenced this issue May 28, 2024
globally defined packages are specified in eslint config

Issue #2096
robyngit added a commit that referenced this issue May 28, 2024
- Run workflow on PRs only to avoid duplicated actions
- Combine all checks into one workflow

Issue #2096
robyngit added a commit that referenced this issue May 28, 2024
- Run workflow on PRs only to avoid duplicated actions
- Combine all checks into one workflow

Issue #2096
robyngit added a commit that referenced this issue May 28, 2024
- Reformat & cleanup the doc as well

Issue #2096
robyngit added a commit that referenced this issue May 28, 2024
- Reformat & cleanup the doc as well

Issue #2096
@robyngit robyngit linked a pull request May 29, 2024 that will close this issue
robyngit added a commit that referenced this issue May 30, 2024
- Initial prettier setup; do not format yet
- Fix syntax errors in files that would prevent prettier from formatting them

Issue #2096
robyngit added a commit that referenced this issue May 30, 2024
globally defined packages are specified in eslint config

Issue #2096
robyngit added a commit that referenced this issue Jun 4, 2024
@robyngit robyngit removed this from the 2.30.0 milestone Jun 4, 2024
@robyngit
Copy link
Member Author

robyngit commented Jun 4, 2024

Released in 2.29.1

@robyngit robyngit closed this as completed Jun 4, 2024
robyngit added a commit that referenced this issue Jun 5, 2024
- Initial prettier setup; do not format yet
- Fix syntax errors in files that would prevent prettier from formatting them

Issue #2096
robyngit added a commit that referenced this issue Jun 5, 2024
- Initial prettier formatting run

Issue #2096
robyngit added a commit that referenced this issue Jun 5, 2024
globally defined packages are specified in eslint config

Issue #2096
robyngit added a commit that referenced this issue Jun 5, 2024
- Run workflow on PRs only to avoid duplicated actions
- Combine all checks into one workflow

Issue #2096
robyngit added a commit that referenced this issue Jun 5, 2024
- Reformat & cleanup the doc as well

Issue #2096
robyngit added a commit that referenced this issue Jun 5, 2024
ianguerin pushed a commit that referenced this issue Jun 5, 2024
- Initial prettier setup; do not format yet
- Fix syntax errors in files that would prevent prettier from formatting them

Issue #2096
ianguerin pushed a commit that referenced this issue Jun 5, 2024
- Initial prettier formatting run

Issue #2096
ianguerin pushed a commit that referenced this issue Jun 5, 2024
globally defined packages are specified in eslint config

Issue #2096
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants