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

Chore: Improve linting & auto-alphabetize translations #2294

Merged
merged 34 commits into from
Dec 23, 2021

Conversation

gutsyphilip
Copy link
Contributor

@gutsyphilip gutsyphilip commented Dec 3, 2021

This PR is a proposal and feature of some sort that contains a series of efforts to help ensure consistency across the codebase. See details below:

Linting and Formatting

For consistency and predictable organization of code, this project uses Eslint for linting and formatting.

The Setup

The configuration for Eslint can be found at the root of this project:

  • Eslint - .eslintrc.js

VSCode is configured to automatically fix fixable linting errors on save. See .vscode/settings.json. You can configure Webstorm or other editors to do the same!

With inpackage.json, there are scripts that you can run to lint, autoFix and format the entire codebase.

"lint": "eslint --ext .js --ext .jsx .",
"fix": "eslint --ext .js --ext .jsx . --fix",

Git Commit hooks

Git commit hooks are a provision that allows us run custom scripts when specific events occur in the git workflow. See more here.

For this project, we are using the clientside commit hooks to automatically lint and alphabetize staged files before they get committed.

Auto-alphabetization of translation files

This is meant to help ensure consistency and ease of reference within the translation files. For this, we are making use of git hooks via a package called husky. Husky allows us to configure a defined pre-commit command which triggers the precommit script within modified packages. The precommit script in packages/frontend/package.json is configured to run lint-staged. Lint-staged allows us run linters on files that have been staged through git.

Within package.json, the block below defines what linters and formatters get applied to what filetype.

 "lint-staged": {
    "*.{js,ts,tsx, jsx}": [
      "eslint --quiet --fix",
    ],
    "src/translations/*.json": "sort-json --ignore-case true"
  }

Two main things are ensured through this configuration:

  • Staged translation files src/translations/*.json" get sorted in alphabetical order automatically using a package called sort-json
  • Staged .js and .jsx files get linted for any major linting errors and committing will fail until linting errors are resolved e.g unused vars. We can make the rules as tight or loose as we want through the configuration in .eslintrc.js.

Proposal: Commit messages

Within .husky/commit-msg, there's a script that ensures consistent formatting with commit messages using the commit-msg git hook.

The proposed format is:

git commit -m "{commit type goes here}: {commit information}"

Example:

git commit -m "feat: linting support using eslint"

The image below provides more information on the commit types.

image

Learn more here. This is not a major thing, however. It just seemed like a somewhat beneficial low-hanging fruit.

Curious mention.

Recently discovered this project : https://trunk.io
Would love to hear your thoughts.

@netlify
Copy link

netlify bot commented Dec 3, 2021

👷 Deploy request for near-wallet-staging pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: b158169

@gutsyphilip gutsyphilip marked this pull request as ready for review December 4, 2021 02:48
@gutsyphilip gutsyphilip changed the title Chore: Improve linting & auto-alphabetize JSON Chore: Improve linting & auto-alphabetize translations Dec 4, 2021
Copy link
Contributor

@esaminu esaminu left a comment

Choose a reason for hiding this comment

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

Great stuff! Minor comments in-line 😄

packages/frontend/package.json Outdated Show resolved Hide resolved
packages/frontend/package.json Outdated Show resolved Hide resolved
packages/frontend/package.json Outdated Show resolved Hide resolved
@esaminu
Copy link
Contributor

esaminu commented Dec 6, 2021

@worldclassdev Please rebase to fix the failing fossa check

Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

This is great stuff, thanks @worldclassdev :) We're missing a lot of dev tooling so this is a step in the right direction :)

Added just a couple of requests in-line -- the main thing is that I'd like to avoid adding Prettier to the project, and instead rely on eslint --fix. This is because prettier will aggressively reformat code based on line-length to the detriment of changeset clarity -- it's easy to make a simple change that reduces the length of a variable name or some other arbitrary and non-behaviour-changing way, but see Prettier create a diff involving a hundred lines of code or more (especially in large hooks based components). In particular because we work with community contributions, being an open source project, I want to avoid tooling that adds this type of churn to PRs and take a little bit of a softer approach to code-style enforcement (eslint is configurable and does not have a 'line-length-above-all-else' type approach to formatting like Prettier does)) :)

packages/frontend/.eslintrc.js Outdated Show resolved Hide resolved
packages/frontend/package.json Outdated Show resolved Hide resolved
.husky/commit-msg Outdated Show resolved Hide resolved
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

@worldclassdev As your first task officially as part of the team (woot! :)), please get this branch in-sync with master again (force pull to overwrite changes to the translation files; they have been edited by other contributors recently), then re-run the re-formatting to re-sort the json files by their keys -- we'll get this landed as soon as they are re-sorted and you've addressed the few in-line comments/requests in this review :)

.husky/commit-msg Outdated Show resolved Hide resolved
packages/frontend/docs/Linting-and-formatting.md Outdated Show resolved Hide resolved
packages/frontend/docs/Linting-and-formatting.md Outdated Show resolved Hide resolved
packages/frontend/docs/Linting-and-formatting.md Outdated Show resolved Hide resolved
@MaximusHaximus MaximusHaximus merged commit b4f2963 into near:master Dec 23, 2021
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