Fix "Add Plot" breaking dvc.yaml when there is no list indentation #5126
Conversation
julieg18
left a comment
julieg18
left a comment
There was a problem hiding this comment.
This solution feels hacky but I'm not seeing a better way 🤔. At least I don't think this route will get hit very often since list items with no indentation are not the norm in my experience.
| const mockPlotYamlContent = [ | ||
| '', | ||
| '- simple_plot:', | ||
| ' template: simple', |
There was a problem hiding this comment.
You can see in the demo (and these tests) that the new plot yaml still doesn't line up perfectly with the previous plots since we're grabbing the indent level from a nested item (x: recall in this example) which doesn't always tell you the plots actual indent level.
Since I'm not seeing an simple way to make sure it's all aligned correctly, I chose to aim for just ensuring that the new yaml is valid and doesn't break anything :)
| line.startsWith('-') | ||
| ) | ||
| return doesYamlListItemHaveNoIndent | ||
| ? newPlotLines.map(line => |
There was a problem hiding this comment.
We check for if the lines have a list item with no indentation and trim our new plot lines to ensure that the new plot also has 0 indentation for its list item.
| ' y:', | ||
| ' eval/prc/train.json: precision', | ||
| ' eval/prc/test.json: precision' | ||
| ].join('\n') |
There was a problem hiding this comment.
You can use string literals to test for format (`) instead of creating lines and joining them.
There was a problem hiding this comment.
Yes, I prefer creating lines since I find it easier to keep track of the exact spacing on each line.
|
Code Climate has analyzed commit 0ef49d6 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.2% (0.0% change). View more on Code Climate. |
2 participants
mainScreen.Recording.2023-12-15.at.10.44.46.AM.mov
PR
Screen.Recording.2023-12-15.at.10.51.29.AM.mov
Fixes #5122