-
Notifications
You must be signed in to change notification settings - Fork 394
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
Document that YAML files are expected to be in YAML1.2 #1764
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.
Looks great overall, I would avoid using notes though
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
YAML, JSON, or TOML _parameters file_. Multiple parameter dependencies can be | ||
specified from one or more parameters files. | ||
YAML (specifically in YAML 1.2 format), JSON, or TOML _parameters file_. | ||
Multiple parameter dependencies can be specified from one or more parameters | ||
files. |
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.
Same, just "YAML 1.2"? (Linked)
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.
Also, there's several other files that include this params file formats list. Please see #1799 (review)
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.
- In fact I'd just grep
YAML
(upper case and double check all instances (there's 28). DONE SEPARATELY
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.
Let's not change everywhere, it's enough to mention it in key areas. It's still YAML, so it's fine.
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.
Same here, just Same, just "YAML 1.2" please.
it's enough to mention it in key areas
OK but why are the metrics/params index more important than docs/start/experiments or docs/user-guide/basic-concepts/parameter? (or other places)
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.
In fact I'd just grep YAML (upper case and double check all instances
I didn't mean change it everywhere BTW, but we need to check all 28 instances please, and decide which ones to change. I think it should definitely be more than just this one
It's a simple change from YAML to "YAML 1.2".
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.
Thanks @skshetry ! Left some specific comments above ☝️
p.s. I can polish the language if needed in the next round.
Also, questions/requests:
|
This comment has been minimized.
This comment has been minimized.
The workaround no longer works, so it's YAML1.2 for now. |
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.
Thanks for the answers @skshetry. No changes still, please see the open discussions above. Thanks!
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Restyle Document that YAML files are expected to be in YAML1.2
Merged. It's not that important change to do many iterations to my mind. |
Refer iterative/dvc#4415
Fixes #1704
❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.
🐛 Please make sure to mention
Fix #issue
(if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.Please choose to allow us to edit your branch when creating the PR.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏