Experiment table open to the side #1796

wolmir · 2022-05-30T18:13:57Z

This PR will add the option Open to the side to experiment table columns that represent parameters. This option will open the corresponding params file in a new side-by-side editor area.

Screen.Recording.2022-05-30.at.16.31.45.mov

mattseddon

👍🏻

LGTM, just need to add an event for open openParamsFileToTheSide. The logic from that function could also be moved out to somewhere in the columns folder but that is just personal preference for me.

mattseddon · 2022-05-30T23:09:20Z

extension/src/experiments/index.ts

@@ -380,6 +382,13 @@ export class Experiments extends BaseRepository<TableData> {
    )
  }

+  private async openParamsFileToTheSide(path: string) {
+    const [, fileSegment] = splitColumnPath(path)
+    await window.showTextDocument(Uri.file(join(this.dvcRoot, fileSegment)), {


[I] Potentially we could rename and re-use RegisteredCommands.TRACKED_EXPLORER_OPEN_TO_THE_SIDE here. If not this should have its own analytics event.

Probably better to just give this its own event.

mattseddon · 2022-05-30T23:12:23Z

webview/src/util/array.ts

@@ -0,0 +1,3 @@
+export function pushIf<T>(array: T[], condition: boolean, elements: T[]) {


[N] We only use function instead of const/arrow function if we have to.

mattseddon · 2022-05-30T23:14:04Z

webview/src/experiments/components/table/TableHeader.tsx

+        }
+      }
+    ]
+


[C] Would be good to get a divider between these two. Same for row context menus, we can follow up though.

sroy3 · 2022-05-31T13:44:48Z

I get the option to open to the side when I right click params.yml, but clicking it does not do anything (it also shows hide column and it does not do anything).

Out of scope, but I also noticed this while reviewing:

wolmir · 2022-05-31T16:21:19Z

I get the option to open to the side when I right click params.yml, but clicking it does not do anything (it also shows hide column and it does not do anything).

Out of scope, but I also noticed this while reviewing:

Thanks, I'll verify it now!

rogermparent

Looks good so far! I decided to manually test non-default param file names to ensure we won't run into issues in the future, and everything looks solid!

Scenarios I tried:

additional param file named otherparams.yaml
nested additional param file names nested/params.yaml
param file name with a space in it
removing the params key entirely from dvc.yaml and relying on default behavior

All of these scenarios work perfectly fine! We may want to add automated tests for these scenarios for future refactors, though.

Aside from that, I confirmed that what @sroy3 pointed out with params.yaml headers also happens on my machine, and think it should be fixed either in this PR or an immediate follow-up, preferably the former since the issue doesn't exist on main.

rogermparent · 2022-05-31T18:22:51Z

extension/src/test/suite/experiments/index.test.ts

+    it('should be able to handle a message to open the source params file from a column path', async () => {
+      const { experiments } = setupExperimentsAndMockCommands()
+
+      const mockShowTextDocument = stub(window, 'showTextDocument')
+      const webview = await experiments.showWebview()
+      const mockMessageReceived = getMessageReceivedEmitter(webview)
+      const mockColumnId = 'params:params.yaml:lr'
+
+      mockMessageReceived.fire({
+        payload: mockColumnId,
+        type: MessageFromWebviewType.OPEN_PARAMS_FILE_TO_THE_SIDE
+      })
+
+      expect(mockShowTextDocument).to.be.calledOnce
+      expect(mockShowTextDocument).to.be.calledWithExactly(
+        Uri.file(join(dvcDemoPath, 'params.yaml')),
+        {
+          viewColumn: ViewColumn.Beside
+        }
+      )
+    })


Could be good to test non-default params file names, just in case we refactor in the future.

Thanks! I already changed this test to use paths with appended numbers like React table does so I can simulate this issue you guys found.
I will also add a test for non-default files now because it's easy to do.

wolmir · 2022-05-31T19:56:23Z

@mattseddon @rogermparent @sroy3
While I was fixing the issue that @sroy3 found I encountered this use case:

To clarify:
I reordered the columns by splitting the params.yaml group and then opened the context-menu.
What should happen when I select the Hide column option?

Hide the entire params.yaml group (same as in the columns tree view)
Hide only the columns in that subgroup of the table

mattseddon · 2022-06-01T00:16:19Z

@mattseddon @rogermparent @sroy3 While I was fixing the issue that @sroy3 found I encountered this use case:

To clarify: I reordered the columns by splitting the params.yaml group and then opened the context-menu. What should happen when I select the Hide column option?

Hide the entire params.yaml group (same as in the columns tree view)

Hide only the columns in that subgroup of the table

I would say definitely option 2 as it would be the least unexpecting thing to do. I would suggest that we limit "Hide Column" to leaves only as the first step.

codeclimate · 2022-06-02T01:18:41Z

Code Climate has analyzed commit a58ed20 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 96.8% (0.0% change).

View more on Code Climate.

wolmir self-assigned this May 30, 2022

wolmir force-pushed the exp-table-open-to-the-side branch 3 times, most recently from 9cfdfa9 to 337bbbb Compare May 30, 2022 19:22

wolmir requested review from mattseddon, sroy3 and rogermparent May 30, 2022 19:30

wolmir marked this pull request as ready for review May 30, 2022 19:30

mattseddon approved these changes May 30, 2022

View reviewed changes

wolmir force-pushed the exp-table-open-to-the-side branch from 337bbbb to 2a8c56e Compare May 31, 2022 02:27

rogermparent reviewed May 31, 2022

View reviewed changes

wolmir force-pushed the exp-table-open-to-the-side branch from cc9c6bb to f45831d Compare May 31, 2022 20:06

wolmir added 7 commits June 1, 2022 11:45

Add Open-to-the-side option to table header context-menu

04a235e

Remove commented code

04d846b

Add test for the Open-to-the-side message handler

a34600d

Add a telemetry event for the open file message

49033bd

Refactor array utils

c734b31

Clean the file segment of column paths before using them

f5cc008

Limit Hide column option to leaves only

e0e6ad8

wolmir force-pushed the exp-table-open-to-the-side branch from f45831d to e0e6ad8 Compare June 1, 2022 14:56

sroy3 approved these changes Jun 1, 2022

View reviewed changes

mattseddon added the product PR that affects product label Jun 2, 2022

mattseddon approved these changes Jun 2, 2022

View reviewed changes

Merge branch 'main' into exp-table-open-to-the-side

8472253

mattseddon enabled auto-merge (squash) June 2, 2022 01:05

mattseddon changed the title ~~Exp table open to the side~~ Experiment table open to the side Jun 2, 2022

mattseddon requested review from maxagin and shcheklein June 2, 2022 01:07

mattseddon disabled auto-merge June 2, 2022 01:07

Merge branch 'main' into exp-table-open-to-the-side

a58ed20

mattseddon merged commit 1d572b5 into main Jun 2, 2022

mattseddon deleted the exp-table-open-to-the-side branch June 2, 2022 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment table open to the side #1796

Experiment table open to the side #1796

wolmir commented May 30, 2022 •

edited

Loading

mattseddon left a comment

mattseddon May 30, 2022

mattseddon May 30, 2022

mattseddon May 30, 2022

mattseddon May 30, 2022

sroy3 commented May 31, 2022

wolmir commented May 31, 2022

rogermparent left a comment •

edited

Loading

rogermparent May 31, 2022

wolmir May 31, 2022

wolmir commented May 31, 2022

mattseddon commented Jun 1, 2022 •

edited

Loading

codeclimate bot commented Jun 2, 2022

		@@ -0,0 +1,3 @@
		export function pushIf<T>(array: T[], condition: boolean, elements: T[]) {

Experiment table open to the side #1796

Experiment table open to the side #1796

Conversation

wolmir commented May 30, 2022 • edited Loading

mattseddon left a comment

Choose a reason for hiding this comment

mattseddon May 30, 2022

Choose a reason for hiding this comment

mattseddon May 30, 2022

Choose a reason for hiding this comment

mattseddon May 30, 2022

Choose a reason for hiding this comment

mattseddon May 30, 2022

Choose a reason for hiding this comment

sroy3 commented May 31, 2022

wolmir commented May 31, 2022

rogermparent left a comment • edited Loading

Choose a reason for hiding this comment

rogermparent May 31, 2022

Choose a reason for hiding this comment

wolmir May 31, 2022

Choose a reason for hiding this comment

wolmir commented May 31, 2022

mattseddon commented Jun 1, 2022 • edited Loading

codeclimate bot commented Jun 2, 2022

wolmir commented May 30, 2022 •

edited

Loading

rogermparent left a comment •

edited

Loading

mattseddon commented Jun 1, 2022 •

edited

Loading