-
Notifications
You must be signed in to change notification settings - Fork 2
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
expand descriptions #16
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
@dberenbaum, there's a |
Sorry! Fixed now. |
gen.py
Outdated
Can be a string or a sequence of commands.""" | ||
|
||
PARAMS_DESC = """\ | ||
Sequence of dot-separated parameter dependency keys to track from `params.yaml`. |
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.
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.
Yeah, it's not going to generate code blocks. Should we replace them with something else? I'm not sure if there's a better way to mark them or if it's fine for them to show as backticks. @jorgeorpinel Maybe you have some thoughts here?
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.
Regular '
or nothing? Can you use HTML? I.e. <code>
tags
@@ -328,7 +328,7 @@ | |||
"properties": { |
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.
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.
Added description for stage
and other subfields that had none.
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.
Hey. Took the liberty to check for consistency with docs. Not sure how important that is for this repo, feel free to take whatever makes sense:
class CustomParamFileKeys(BaseModel): | ||
__root__: Dict[FilePath, Set[ParamKey]] | ||
__root__: Dict[FilePath, Set[ParamKey]] = Field( | ||
..., desc="Path to YAML/JSON/TOML/Python params file." |
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 specify YAML 1.2 and TOML 1.0 in DVC docs. Probably too much info for here though? Just saying
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.
Left this out for brevity.
PLOT_DESC = """\ | ||
Path to plots file or dir of the stage. |
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 don't think you can use directories for stage plots. Can you?
At least it's not in the docs.
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.
Yup, you can! 🤔 I don't actually see it in either type of plots. I can open a PR.
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 don't think you can use directories for stage plots
Yup, you can! 🤔 I don't actually see it in either type of plots. I can open a PR.
That would be great, thanks! 🙏🏼
gen.py
Outdated
PARAMS_DESC = """\ | ||
Sequence of dot-separated parameter dependency keys to track from `params.yaml`. | ||
|
||
May contain other YAML/JSON/TOML/Python parameter file names, with a \ | ||
sub-sequence of the param names to track in them (leave empty to include all).\ |
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'd simplify. This tries to be super formal but may end up confusing people or not being understood. Can we link to the docs instead? E.g.
PARAMS_DESC = """\ | |
Sequence of dot-separated parameter dependency keys to track from `params.yaml`. | |
May contain other YAML/JSON/TOML/Python parameter file names, with a \ | |
sub-sequence of the param names to track in them (leave empty to include all).\ | |
PARAMS_DESC = """\ | |
List of parameter names (or groups) to track from `params.yaml`. | |
May be plain strings or dot-separated tree paths. | |
Other YAML/JSON/TOML/Python param files may be used | |
by prefixing their file paths to list elements (with `:`). | |
See | |
https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#parameters | |
for details. |
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 can simplify, but I don't know that it's possible to create actionable links.
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 don't mind the copy edits, but again wonder why/where they come from. The language was largely borrowed from https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#stage-entries with some additional technical info added like file formats.
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.
Just trying to use more general language e.g. avoid "sequence/sub-sequence" and "key" (which we don't use much in docs either).
Not critical if this is not user-facing, but it's good to have consistent terminology with docs when possible I think?
@jorgeorpinel I already merged but meant to leave a general comment: I prioritized consistency with the docs and incorporated any of your suggestions to that effect. Thanks for the review! |
Related to iterative/vscode-dvc#1192 (showing more helpful descriptions in VS Code).