Skip to content

Conversation

@Suor
Copy link
Contributor

@Suor Suor commented May 29, 2020

Along the way:

  • several targets in dvc plots diff now work
  • fixed inconsistency between CLI and Repo API (x vs x_field, xlab and x_title)
  • fixes some wrong error handling

NOTE: several plots per plot data file not supported here. There are at least two design decisions need to be made: how do they interact with dvc plots add/modify/delete and how do we collect them when they are inconsistent.

Collecting inconsistent props from different revisions for now solved by using props from first rev mentioned and issuing a warning.

@Suor Suor requested a review from efiop May 29, 2020 16:15
@Suor
Copy link
Contributor Author

Suor commented May 29, 2020

There is no tests for new dvc plots modify command here, but existing tests pass. Will need to merge it anyway so that @dmpetrov will be able to generate a new test repo for viewer.

Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>
Suor and others added 4 commits May 30, 2020 15:34
Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>
Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>
Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>
Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>
@efiop
Copy link
Contributor

efiop commented May 30, 2020

Looks good! Let's merge and work on top. Thanks @Suor ! 🙏

@efiop efiop merged commit 516d82e into treeverse:master May 30, 2020
@pared pared mentioned this pull request Jun 5, 2020
_add_output_arguments(plots_diff_parser)
plots_diff_parser.set_defaults(func=CmdPlotsDiff)

PLOTS_MODIFY_HELP = "Modify plot props associated with a target file."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we call them "plot display properties" or at least "properties" in user-facing strings please? 🙂

Lmk if so to change it in treeverse/dvc.org/pull/1382 while it's in progress, to avoid requiring another docs PR for such change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgeorpinel created #3983 .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated docs PR too.

@efiop
Copy link
Contributor

efiop commented Jun 8, 2020

@Suor Could you take a look at this one too, please?

@efiop efiop mentioned this pull request Jun 8, 2020
3 tasks
efiop pushed a commit to efiop/dvc that referenced this pull request Jun 8, 2020
efiop added a commit that referenced this pull request Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants