-
Notifications
You must be signed in to change notification settings - Fork 50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!! ⚡️
Inlined some wording suggestions.
Co-Authored-By: Rūdolfs Ošiņš <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, @xla!
Just a few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great so far! Just 2 suggestions
yarn prettier:check # Check UI code formatting | ||
yarn prettier:write # Auto-format UI code | ||
yarn lint # Check UI code for linting errors | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have a bunch of helper scripts for the proxy in our package.json
, should we maybe include those here too with proper descriptions?
proxy:build # cd proxy && cargo build --all-features --all-targets
proxy:build:release # cd proxy && cargo build --release --all-features --all-targets
proxy:build:test # cd proxy && cargo test --no-run
proxy:start # cd proxy && cargo run --release -- --registry=emulator
proxy:start:test # cd proxy && cargo run -- --registry=emulator --test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally I find these outputs of cli options to be prone to quickly get outdated. I'd rather show a command which shows all the commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what you mean. This section expands a bit on the commands however. It's meant to explain when they should be used. As the yarn run
command can only show the commands themselves, but there is no way to inline documentation in package.json
.
I just provided the # cd proxy && cargo run
comments as a reference, the idea was that you'd write the explanation in a comment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 4b037b7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments and suggestions about writing style, but for the most part this is a welcome expansion on the norms and procedures of working on the app/proxy.
Co-Authored-By: Merle Breitkreuz <[email protected]> Co-Authored-By: Rūdolfs Ošiņš <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final touches.
Also could we wrap the paragraphs at 80 characters? VIM's gq
is quite handy for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Closes #191 * Correct flag * Apply suggestions from @rudolfs Co-Authored-By: Rūdolfs Ošiņš <[email protected]> * Apply suggestions from code review Co-Authored-By: Merle Breitkreuz <[email protected]> Co-Authored-By: Rūdolfs Ošiņš <[email protected]> * Link downstream protocol projects * Improve languag * Correct position of test disclaimerj * Add proxy commands with explanations * Address feedback Co-authored-by: Rūdolfs Ošiņš <[email protected]> Co-authored-by: Merle Breitkreuz <[email protected]>
Closes #191