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

Add global formatting for the webapp. #130

Merged
merged 10 commits into from
Mar 3, 2024
Merged

Add global formatting for the webapp. #130

merged 10 commits into from
Mar 3, 2024

Conversation

ultraviolet10
Copy link
Collaborator

@ultraviolet10 ultraviolet10 commented Feb 27, 2024

Adds a global format command that runs prettier's --write on all specified file types.

Uses the following options within the config (prettierrc.json):

Also added eslint-config-prettier to be used as a plugin as the recommended mediator between linting and formatting.

@ultraviolet10 ultraviolet10 self-assigned this Feb 27, 2024
@ultraviolet10 ultraviolet10 changed the title Add global formatting (Prettier). Add global formatting for the webapp. Feb 27, 2024
@ultraviolet10
Copy link
Collaborator Author

ultraviolet10 commented Mar 1, 2024

there's another interesting plugin we could use: https://www.npmjs.com/package/eslint-plugin-simple-import-sort

this helps sort imports in different files according to your specs, here's an example one:

"simple-import-sort/imports": [
      "error",
      {
        "groups": [
          // Packages. `react` related packages come first.
          [
            "^react",
            "^next",
            "@metamask|@walletconnect|clsx|@reduxjs|async-mutex|flowbite-react|redux-persist|pubnub|redux",
            "^(ethers|google|lodash|bn.js|styled-components|child_process|cluster|console|constants|crypto|dgram|dns|domain|events|fs|http|https|module|net|os|path|punycode|querystring|readline|repl|stream|string_decoder|sys|timers|tls|tty|url|util|vm|zlib|freelist|v8|process|async_hooks|http2|perf_hooks)"
          ],
          // Internal packages.
          ["^@?\\w"],
          // Parent imports. Put `..` last.
          ["^\\.\\.(?!/?$)", "^\\.\\./?$"],
          // Other relative imports. Put same-folder imports and `.` last.
          ["^\\./(?=.*/)(?!/?$)", "^\\.(?!/?$)", "^\\./?$"],
          // Style imports.
          ["^.+\\.s?css$"],
          // Side effect imports.
          ["^\\u0000"]
        ]
      }

@ultraviolet10
Copy link
Collaborator Author

@norswap
Copy link
Member

norswap commented Mar 1, 2024

"tabWidth": 2: @norswap did you want this as 4?

Yes let's go for 4!

"useTabs": true: https://prettier.io/docs/en/options.html#tabs

This is always religious, but I do prefer spaces myself. I guess with prettier it matters less, but it ensures proper display on Github for instance.

Any counter here? Like would you strongly prefer a 2 or 8 spaces local display?

.github/workflows/check.yaml Outdated Show resolved Hide resolved
- name: Install dependencies for webapp
run: cd packages/webapp && pnpm install
- name: Check formatting with Prettier
run: cd packages/webapp && npx prettier --check "**/*.{js,jsx,ts,tsx,json,css}"
Copy link
Member

Choose a reason for hiding this comment

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

This should be folded into make check in the webapp Makefile (which will cause it be checked by the global Makefile make check and thus covered by the above)

Makefile Outdated
# Performs code formatting for the webapp files and contracts in their respective directories.
format:
cd packages/webapp && make format
cd packages/contract && make check
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cd packages/contract && make check
cd packages/contract && make format

# Runs prettier formatting across webapp files with specified file extensions.
format:
npx prettier --write "**/*.{js,jsx,ts,tsx,json,css}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
npx prettier --write "**/*.{js,jsx,ts,tsx,json,css}"
pnpm prettier --write "**/*.{js,jsx,ts,tsx,json,css}"

@ultraviolet10
Copy link
Collaborator Author

"tabWidth": 2: @norswap did you want this as 4?

Yes let's go for 4!

"useTabs": true: https://prettier.io/docs/en/options.html#tabs

This is always religious, but I do prefer spaces myself. I guess with prettier it matters less, but it ensures proper display on Github for instance.

Any counter here? Like would you strongly prefer a 2 or 8 spaces local display?

No hard preference here, 4 is good!

@ultraviolet10 ultraviolet10 requested a review from norswap March 2, 2024 11:23
Copy link
Member

@norswap norswap left a comment

Choose a reason for hiding this comment

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

Approving, but address comments!

@@ -20,4 +21,5 @@ jobs:
run: mkdir -p packages/contracts/out && echo "{}" > packages/contracts/out/deployment.json
- run: make setup
- run: make check

- name: Check formatting with Prettier
run: cd packages/webapp && make check
Copy link
Member

Choose a reason for hiding this comment

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

already covered by make check above, this file shouldn't be changed basically

(Sorry, I think I recall I said this needed to be updated, but if make check checks the format, it's not needed!)

Copy link
Member

Choose a reason for hiding this comment

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

isn't 150 too much? I like 100 or 120

check:
pnpm eslint . && pnpm prettier --check "**/*.{js,jsx,ts,tsx,json,css}"
.PHONY: check
Copy link
Member

Choose a reason for hiding this comment

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

you can put both of these on different lines (no need for &&)

@ultraviolet10 ultraviolet10 merged commit 639f9ff into master Mar 3, 2024
1 check failed
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.

2 participants