-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Remove CLI option true defaults to enable negation #5582
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.
Did you ensure that all existing instances of DocgenOptions still choose the same values as before?
Are you referring to instances in other files, like build_options: BuildOptions {
with_srcs,
with_abis: true,
with_source_maps: false,
with_error_map: true,
named_addresses: Default::default(),
install_dir: None,
with_docs: true,
docgen_options: Some(DocgenOptions {
include_impl: true,
include_specs: true,
specs_inlined: false,
include_dep_diagram: false,
collapsed_sections: true,
landing_page_template: Some("doc_template/overview.md".to_string()),
references_file: Some("doc_template/references.md".to_string()),
}),
}, Would these also need to be updated? Alternatively, if you could make it so the CLI accepts a negation modifier on an option that would rectify the problem too. The real issue here is that there seems to be no way in practice to negate the options that were defaulted to true, which I modified in the commit. Is there something I'm missing here? |
/// Whether to include specifications in the generated documentation. Defaults to true. | ||
#[clap(long, global = true)] | ||
/// Whether to include specifications in the generated documentation. Defaults to false. | ||
#[clap(long)] | ||
pub include_specs: bool, |
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.
The other option is we can make it exclude_specs. This still lets it default to true, but allows for negation.
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.
It is probably fine.
Just want to be sure that there is no behavioral change but this case looks OK to me, so I think we are fine. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@wrwg @davidiw @gregnazario @alinush Per aptos-labs#5581 , DocGen CLI options defaulting to true prohibit negation.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
@wrwg @davidiw @gregnazario @alinush Per aptos-labs#5581 , DocGen CLI options defaulting to true prohibit negation.
@wrwg @davidiw @gregnazario @alinush
Per #5581 , DocGen CLI options defaulting to true prohibit negation.
This change is