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

Follow root level naming convention as proposed in #977 #1006

Merged
merged 6 commits into from
Dec 12, 2024

Conversation

schmidsi
Copy link
Contributor

@schmidsi schmidsi commented Dec 4, 2024

Description

Changes root-level commands names to follow the {package}:{command} standard. Except some globally important commands: yarn chain, yarn deploy, yarn start.

Additional Information

Related Issues

Closes #977

Your ENS/address: ses.eth

@schmidsi
Copy link
Contributor Author

schmidsi commented Dec 4, 2024

It might be worth having a discussion about the other global commands. Here is my argumentation why I did not include other commands:

Generally, the root commands should:
a) Do root-level stuff like postinstall or precommit, or
b) Do stuff on multiple packages like test or format, or
c) Be part of the necessary happy path like chain, deploy, start

  • "account" -> This is not necessarily needed. Could be seen as needed for chain deploys.
  • "compile" -> Not needed, deploy also compiles
  • "flatten" -> That is usually an edge-case for verification.
  • "fork" -> Special use-case, not part of happy path
  • "generate" -> Not needed, deploy also generates types and stuff
  • "verify" -> Not part of happy path since there are multiple ways to verify contracts

@technophile-04
Copy link
Collaborator

Thanks @schmidsi! This canges makes sense! A coule of things which I feel we should do is:

  1. Remove the echo 'Deprecated messages in happy path commands. I think it's not worth it.
  2. Add generate, account, and verify, fork to happy path commands
  3. Remove yarn compile, yarn flatten , yarn hardhat-verify. Instead we only have hardhat:<command> for all this commands
  4. We sort the commands, Maybe the order could be:
    1. Happy path commands first (chain, start, deploy, verify, generate, account, fork, vercel, vercel:yolo)
    2. commands that do stuff on multiple packages like test or format
    3. package specific command, maybe we keep all the hardhat: first
    4. All the next:commands second
    5. misc like postInstall, precommit.

@rin-st
Copy link
Member

rin-st commented Dec 5, 2024

Thanks @schmidsi !

I agree with @technophile-04 mostly

  1. We sort the commands, Maybe the order could be:

For my eye there will be no difference between i and ii, and also it requires remembering where i, where ii and where v

For me, alphabetical order for everything is good since it's easier for an eye to search the script and doesn't require to remember the order of groups

The second option could be

  1. hardhat:<command> group
  2. next:<command> group
  3. everything else in alphabetical order

it almost doesn't require to remember order of groups too

@schmidsi
Copy link
Contributor Author

schmidsi commented Dec 5, 2024

I personally like the alphabetical ordering too: It's easy to reason about, like no discussion about "does this belong to happy path or misc"? Also, it looks pretty good and it groups the prefixed ones. This is what it looks like on my test repo:

"scripts": {
    "account": "yarn hardhat:account",
    "chain": "yarn hardhat:chain",
    "deploy": "yarn hardhat:deploy",
    "fork": "yarn hardhat:fork",
    "format": "yarn next:format && yarn hardhat:format",
    "generate": "yarn hardhat:generate",
    "graph": "yarn workspace @se-2/subgraph graph",
    "graphclient:build": "yarn workspace @se-2/nextjs client",
    "hardhat:account": "yarn workspace @se-2/hardhat account",
    "hardhat:chain": "yarn workspace @se-2/hardhat chain",
    "hardhat:check-types": "yarn workspace @se-2/hardhat check-types",
    "hardhat:compile": "yarn workspace @se-2/hardhat compile",
    "hardhat:deploy": "yarn workspace @se-2/hardhat deploy",
    "hardhat:flatten": "yarn workspace @se-2/hardhat flatten",
    "hardhat:fork": "yarn workspace @se-2/hardhat fork",
    "hardhat:format": "yarn workspace @se-2/hardhat format",
    "hardhat:generate": "yarn workspace @se-2/hardhat generate",
    "hardhat:hardhat-verify": "yarn workspace @se-2/hardhat hardhat-verify",
    "hardhat:lint": "yarn workspace @se-2/hardhat lint",
    "hardhat:lint-staged": "yarn workspace @se-2/hardhat lint-staged",
    "hardhat:test": "yarn workspace @se-2/hardhat test",
    "hardhat:verify": "yarn workspace @se-2/hardhat verify",
    "postinstall": "husky install",
    "next:build": "yarn workspace @se-2/nextjs build",
    "next:check-types": "yarn workspace @se-2/nextjs check-types",
    "next:format": "yarn workspace @se-2/nextjs format",
    "next:lint": "yarn workspace @se-2/nextjs lint",
    "next:serve": "yarn workspace @se-2/nextjs serve",
    "precommit": "lint-staged",
    "start": "yarn workspace @se-2/nextjs dev",
    "subgraph:abi-copy": "yarn workspace @se-2/subgraph abi-copy",
    "subgraph:clean-node": "yarn workspace @se-2/subgraph clean-node",
    "subgraph:codegen": "yarn workspace @se-2/subgraph codegen",
    "subgraph:create-local": "yarn workspace @se-2/subgraph create-local",
    "subgraph:local-ship": "yarn workspace @se-2/subgraph local-ship",
    "subgraph:run-node": "yarn workspace @se-2/subgraph run-node",
    "subgraph:stop-node": "yarn workspace @se-2/subgraph stop-node",
    "subgraph:test": "yarn workspace @se-2/subgraph test",
    "test": "yarn hardhat:test",
    "vercel": "yarn workspace @se-2/nextjs vercel",
    "vercel:yolo": "yarn workspace @se-2/nextjs vercel:yolo",
    "verify": "yarn hardhat:verify"
  },

@technophile-04 would you agree? Otherwise, let me know, I can also change to your proposed ordering. I don't have a strong opinion here.

@schmidsi
Copy link
Contributor Author

schmidsi commented Dec 5, 2024

It's probably also easier to compose multiple commands from multiple plugins together if sorted alphabetically.

Copy link
Member

@carletex carletex 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 looking sharp @schmidsi

I think sorting alphabetically makes sense, so we don't have to think where to add new stuff.

A couple of things:

  1. I'd personally miss the yarn compile happy path, but maybe it's just me.
  2. This is probably for another PR/discussion, but: In the past, we've talked about having a global yarn lint command (lint everything + check-types). Same as yarn lint-staged but for the current state of your files (not only staged ones).

@schmidsi
Copy link
Contributor Author

schmidsi commented Dec 5, 2024

@carletex, thanks for the feedback. Added both. +1 for a global lint command.

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

I think this looks great, Thanks!! A very small nitpick maybe we update "scripts" section of packages/nextjs/package.json and packages/hardhat/package.json too? Just so that everything follows same rule...very small thing but lol not sure in future when touch again the scripts sections of packages.json, so that's why trying to get it in this PR 🙌

@schmidsi schmidsi force-pushed the command-naming-convention branch from 5a7efa7 to 2c16b7e Compare December 11, 2024 13:56
@schmidsi
Copy link
Contributor Author

@technophile-04 good point. I've sorted those alphabetical too. Rebased, force-pushed, let's merge that mfer in ;)

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Thanks @schmidsi! Looks great!

@technophile-04 technophile-04 merged commit 74d2baa into scaffold-eth:main Dec 12, 2024
1 check passed
@schmidsi schmidsi deleted the command-naming-convention branch December 12, 2024 13:46
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.

4 participants