-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Change forc doc --manifest-path to forc doc --path #6109
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.
Hello, thanks for the PR. It seems like you just updated docs and readme, not the command itself.
sway/forc-plugins/forc-doc/src/cli.rs
Line 26 in 24d03f4
pub manifest_path: Option<String>, |
I understand correctly that I have to update |
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 would prefer to make --path
a shorthand for --manifest-path
(keeping both as valid CLI flags) so as not to break any existing users.
So you want to keep both run as valid? I'm not sure if the duplicates will lead to good code performance, but if you think so, then I'll do it. |
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 would prefer to make
--path
a shorthand for--manifest-path
(keeping both as valid CLI flags) so as not to break any existing users.So you want to keep both run as valid?
run: forc doc --manifest-path ./sway-lib-std
run: forc doc --path ./sway-lib-std
I'm not sure if the duplicates will lead to good code performance, but if you think so, then I'll do it.
It shouldn't affect the code performance at all. You can use clap's alias:
#[clap(long, alias="manifest-path")]
pub path: Option<String>,
Please fix the compilation errors. You need to update manifest_path
in the other places it is used.
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.
Current ci.yml file is invalid, I suggested the correct version of it. But also I think we should not test whole sdk-harness twice just to see that a flag is working with different alias to preserve some valuable CI time. Let's keep the addition to the code to support it for users but remove it from ci. I think @sdankel originally wanted you to add the support to the code not to CI but pinging her in any case.
Co-authored-by: Kaya Gökalp <[email protected]>
Co-authored-by: Kaya Gökalp <[email protected]>
#[clap(long)] | ||
pub manifest_path: Option<String>, | ||
pub path: Option<String>, |
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 can probably use @sdankel's suggestion here:
#[clap(long)] | |
pub manifest_path: Option<String>, | |
pub path: Option<String>, | |
#[clap(long, alias="manifest-path")] | |
pub path: Option<String>, |
I absolutely agree with @kayagokalp , we should not run forc-doc twice in CI. This code still doesn't compile, so I'm going to close this PR. This isn't really close to what we're looking for and I want to leave the opportunity for another contributor to pick up this task. |
Description
Closes #5789
Checklist
Breaking*
orNew Feature
labels where relevant.