Conversation
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Just some small things, but those can get fixed later. Looks good!
| "js:format": "tsx ./scripts/js.mts format clients/js", | ||
| "js:lint": "tsx ./scripts/js.mts lint clients/js", | ||
| "js:publish": "tsx ./scripts/js.mts publish clients/js", | ||
| "js:test": "tsx ./scripts/js.mts test clients/js", | ||
| "rust:format": "tsx ./scripts/rust.mts format clients/rust", | ||
| "rust:lint": "tsx ./scripts/rust.mts lint clients/rust", | ||
| "rust:lint:clippy": "tsx ./scripts/rust.mts lint-clippy clients/rust", | ||
| "rust:lint:docs": "tsx ./scripts/rust.mts lint-docs clients/rust", | ||
| "rust:lint:features": "tsx ./scripts/rust.mts lint-features clients/rust", | ||
| "rust:publish": "tsx ./scripts/rust.mts publish clients/rust", | ||
| "rust:test": "tsx ./scripts/rust.mts test clients/rust", |
There was a problem hiding this comment.
Is there a reason that these aren't prefixed with clients? Ie. clients:js:format? The reason I ask is that it'll be inconsistent with the other repos
There was a problem hiding this comment.
I think this was the new "template" that we agreed, dropping the "client". Only the system repository is following this at the moment, so we could revert this pattern.
There was a problem hiding this comment.
Hmm yeah I think this is something that wasn't obvious with the system repo because the program doesn't live in the repo but I agree with Jon, this lack consistency and isn't predictable.
I really like our latest refactoring where, for instance, everything Rust-related is in its own script and then you have sub commands within that script.
Maybe we should lean into that more in the pnpm scripts as well. We could have a simple rust script and let the user of that script to provide the arguments.
So instead of something like pnpm program:format we would do pnpm rust format program. That way, there are less "indirections" between the scripts and how they are invoked. What do you think?
There was a problem hiding this comment.
The simplicity of running the commands is definitely nice, but on the flip-side you lose the "self-documenting" nature of package.json, which currently tells you everything that you should be able to run successfully in a repo.
I don't have a very strong preference, but I would stick with the pnpm program:format solely for self-documentation.
| "program:build": "tsx ./scripts/rust.mts build-sbf program --features bpf-entrypoint", | ||
| "program:format": "tsx ./scripts/rust.mts format program", | ||
| "program:lint": "tsx ./scripts/rust.mts lint program", | ||
| "program:lint:clippy": "tsx ./scripts/rust.mts lint-clippy program", | ||
| "program:lint:docs": "tsx ./scripts/rust.mts lint-docs program", | ||
| "program:lint:features": "tsx ./scripts/rust.mts lint-features program", | ||
| "program:test": "tsx ./scripts/rust.mts test program", |
There was a problem hiding this comment.
Is there a reason this is inconsistent with the other repos which prefix with programs instead of program? I'm asking because there are monorepo jobs which test downstream repos, and it's expecting to be able to do programs:build in all them, for example
There was a problem hiding this comment.
Same here, I think we renamed to program in the last refactoring since we usually have a single program per repository.
3 participants
This PR updates the scripts of the repository. It uses the "refactored" ones similar to the System repository.