Add config for updating max amount of table head layers #2436
Conversation
julieg18
changed the title
| @@ -15,7 +15,7 @@ export const limitAncestorDepth = ( | |||
| concatenated layer itself counts as one; because of this, we must subtract 3 | |||
| from what we want the final layer count to be. | |||
| */ | |||
| const convertedLimit = limit - 3 | |||
| const convertedLimit = (limit >= 3 ? limit : 3) - 3 | |||
There was a problem hiding this comment.
Currently, this function only works correctly if the limit is at least 3. There's probably a way to rewrite the logic to allow for 1 or 2 layers though. Should this be done in a followup?
|
There was a problem hiding this comment.
Can we update the max amount of table head layers without closing and reopening the extension?
Fixed now! The table head layers will now update once you change the config without needing to restart :)
With some comments now resolved, I think we're ready for another round of reviews cc @mattseddon
README.md
Outdated
| @@ -150,6 +150,7 @@ These are the VS Code [settings] available for the Extension: | |||
| | -------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | | |||
| | `dvc.dvcPath` | Path or shell command to the DVC binary. Required unless Microsoft's [Python extension] is installed and the `dvc` package found in its environment. | | |||
| | `dvc.pythonPath` | Path to the desired Python interpreter to use with DVC. Required when using a virtual environment. | | |||
| | `dvc.expTableHeadMaxLayers` | Maximum amount of experiment table head rows (minimum is 3). | | |||
There was a problem hiding this comment.
[I] Change amount and layers to depth
There was a problem hiding this comment.
Also, remember to remove minimum when you follow up.
| const convertedLimit = limit - 3 | ||
| const collectedLimit = | ||
| Number(getConfigValue(ConfigKey.EXP_TABLE_HEAD_MAX_LAYERS)) || | ||
| INITIAL_TABLE_HEAD_MAX_LAYERS |
There was a problem hiding this comment.
[C] We can probably get rid of INITIAL_TABLE_HEAD_MAX_LAYERS as the config value should never be undefined (default is 5).
There was a problem hiding this comment.
The tests should
- Reflect the default if no value is provided.
- Test that changing the config value changes the header.
- Confirm that values of 0,1,2 do not break anything.
See
vscode-dvc/extension/src/setup.test.ts
Line 296 in f5748d1
When you follow up to add ability to concatenate to headers to 1 (i.e just leafs as per experiment table) 0 should be the setting for unlimited.
There was a problem hiding this comment.
The tests should
- Reflect the default if no value is provided.
- Test that changing the config value changes the header.
- Confirm that values of 0,1,2 do not break anything.
Completely forgot about updating the tests! Will update in a followup!
|
Code Climate has analyzed commit d7c1103 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 92.3% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.9%. View more on Code Climate. |
Demo
https://user-images.githubusercontent.com/43496356/191370315-9c752b6c-72a7-4610-ae98-89d3b218ddc7.movScreen.Recording.2022-09-22.at.4.49.36.PM.mov
Part of #2390