Skip to content
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

Show plots errors in plots tree #3522

Merged
merged 1 commit into from
Apr 2, 2023
Merged

Show plots errors in plots tree #3522

merged 1 commit into from
Apr 2, 2023

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Mar 21, 2023

3/4 main <- #3477 <- #3520 <- this <- #3532

Relates to #2277

Demo

Screen.Recording.2023-03-21.at.7.21.53.pm.mov

Note: The above shows that I need to add the "full update" logic for when the dvc.yaml is changed (will be done in #3532).

@mattseddon mattseddon added the product PR that affects product label Mar 21, 2023
@mattseddon mattseddon self-assigned this Mar 21, 2023
@mattseddon mattseddon changed the base branch from main to use-errors March 21, 2023 08:37
import { ErrorDecorationProvider } from '../../tree/errorDecorationProvider'
import { uniqueValues } from '../../util/array'

export class DecorationProvider extends ErrorDecorationProvider {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is identical to extension/src/repository/model/decorationProvider.ts and close to extension/src/experiments/model/decorationProvider.ts I can follow up.

@mattseddon mattseddon changed the base branch from use-errors to jump-plots-diff-bc March 22, 2023 00:40
@mattseddon mattseddon changed the base branch from jump-plots-diff-bc to use-errors March 22, 2023 00:40
@mattseddon mattseddon force-pushed the use-errors branch 3 times, most recently from d627859 to f2eda8c Compare March 22, 2023 02:50
@mattseddon mattseddon changed the base branch from use-errors to main March 22, 2023 03:05
@mattseddon mattseddon changed the base branch from main to use-errors March 22, 2023 03:05
@mattseddon
Copy link
Member Author

Note: there is a bug where SCM file decorations can run into the plots tree. I should be able to stop this from happening but it will require a workaround. E.g

image

@mattseddon mattseddon marked this pull request as ready for review March 22, 2023 19:09
@@ -90,6 +92,9 @@ export abstract class BasePathSelectionTree<
if (description) {
treeItem.description = description
}
if (tooltip) {
treeItem.tooltip = tooltip
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of checking for the element before assigning it? If tooltip is undefined, treeItem.tooltip is undefined as well anyway, no? I'm asking because I'm probably missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, there is no point. I'll update

@mattseddon mattseddon enabled auto-merge (squash) April 2, 2023 01:51
@codeclimate
Copy link

codeclimate bot commented Apr 2, 2023

Code Climate has analyzed commit 82fa5b4 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 90.5% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.1% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit 1e7f21c into main Apr 2, 2023
@mattseddon mattseddon deleted the show-tree-errors branch April 2, 2023 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants