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

refactor: Improve script reuse #31

Merged
merged 19 commits into from
Dec 19, 2024
Merged

refactor: Improve script reuse #31

merged 19 commits into from
Dec 19, 2024

Conversation

febo
Copy link
Contributor

@febo febo commented Dec 8, 2024

Problem

There is too much duplication on the use of scripts. For example, there are 2 "versions" of each Rust specific script: one for the client and one for the interface.

Solution

** UPDATED **

The organization of the scripts is simplified by having multiple copies of the same script. Rust scripts expect a relative crate path, so the same script can be used to perform actions on the different crates. Further customization is possible by passing specific command line arguments.

The refactored scripts structure is as follows:

scripts
  |
  +- ci
  |
  +- helpers
  |
  +- js
  |
  +- rust

Since a program usually have multiple Rust crates, scripts for Rust actions expect a relative crate path – e.g., clients/rust (for the Rust client crate), interface (for the interface crate) and program (for the program crate):

"interface:format": "tsx ./scripts/rust/format.mts interface"

Copy link
Contributor

@buffalojoec buffalojoec 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 is looking nice and definitely heading in the right direction, I'm just a little worried about the scripts code continuing to swell. We're almost at the point where we need usage details (ie. --help).

I noticed the default args, like for example with clippy, are becoming increasingly obscured with this layout.

I wonder if there's an approach we can take here where we can condense to just a few scripts, maybe some to do test validator, client generation, and publishing, and then a single js.mjs and rust.mjs for any library operations. For example, these two lines are used across almost every Rust script:

const [env, cliArguments] = getCrateEnvironmentFromArgs();
const args = env.getTestArguments(cliArguments);

Maybe we could have a single JSON or YAML file where you can define default arguments, which can maybe be overridden on the command line. This could be a single file that captures all of the default args defined in the crate folder, and each script could check for an entry in this declaration file during execution.

Then you can just invoke the singular script (rust.mjs or js.mjs) with:

  1. The command
  2. The library path
  3. Flags/arguments

I guess something like:

rust:
  lint:
    interface: [
          "--deny=warnings",
          "--deny=clippy::default_trait_access",
          "--deny=clippy::arithmetic_side_effects",
          "--deny=clippy::manual_let_else",
          "--deny=clippy::used_underscore_binding",
        ]
    program: [
          # ...
        ]

This way, the default args are crystal-clear and easy to configure, and then everything else in the scripts starts to look kind of similar except for the actual commands that are invoked. I'd bet you could wrap much of the setup steps from each script into a single setup, and then key on the provided command argument to handle the rest, within the same script.

What do you think?

scripts/crate/index.mts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
scripts/crate/index.mts Outdated Show resolved Hide resolved
scripts/helpers/rust/wasm.mts Outdated Show resolved Hide resolved
scripts/helpers/js/format.mts Outdated Show resolved Hide resolved
Copy link
Member

@lorisleiva lorisleiva 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 we still have ways to go to find the right balance between offering helpful versus overwhelming scripts but I do believe this is a step in the right direction.

scripts/crate/client.mts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
scripts/crate/index.mts Outdated Show resolved Hide resolved
scripts/crate/index.mts Outdated Show resolved Hide resolved
scripts/crate/interface.mts Outdated Show resolved Hide resolved
scripts/crate/program.mts Outdated Show resolved Hide resolved
scripts/helpers/js/format.mts Outdated Show resolved Hide resolved
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

I'm a big fan of all the helpers/ scripts, since that's exactly what we want for language-specific tasks.

For the configuration, I'm leaning toward @buffalojoec's thinking. Are there any situations in which we have different args for the same job on different packages? For example, if I'm linting a rust package, when will I need different args than the defaults? If I truly need to do something different, then my top-level package.json can specify the different command to run.

That package.json file is essentially the CLI / --help output, since it can list all of actions that you might want to run.

If all the helpers just take a manifest path, or even more general, a directory, then I think we can remove all of the scripts/crate/*. But maybe I'm missing a use-case. I'm also happy to hear any other opinions!

@febo febo marked this pull request as ready for review December 17, 2024 02:12
Copy link
Member

@lorisleiva lorisleiva left a comment

Choose a reason for hiding this comment

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

Love it! Jon was right, there's no need for crate variables at all. 🙌

@buffalojoec
Copy link
Contributor

Nice, looking much smoother with the --manifest-path option.

Let me know what you think of this. I don't think a single script looks all that bad IMO.
#32

@febo
Copy link
Contributor Author

febo commented Dec 18, 2024

Nice, looking much smoother with the --manifest-path option.

Let me know what you think of this. I don't think a single script looks all that bad IMO. #32

I like #32 a lot. We should do the same with the js one, so we don't need the js directory either.

@febo febo merged commit f1c7cc1 into main Dec 19, 2024
8 checks passed
@febo febo deleted the febo/refactor-scripts branch December 19, 2024 14:02
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