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

Group files around concepts #694

Merged
merged 21 commits into from
Jul 28, 2021
Merged

Conversation

mattseddon
Copy link
Contributor

@mattseddon mattseddon commented Jul 28, 2021

6/6 master <- #689 <- #690 <- #691 <- #692 <- #693 <- this

This PR groups files around concepts instead of function.

We now have the following folder structure:

image

image

image

In order to get here I've split the existing extension/src/experiments/quickPick.ts into its component parts and moved the respective tree views. I have also renamed the Runs tree to simply Experiments as per the existing design.

I realise that this is a big change and it would be pretty difficult to integrate with #687. I'm happy to hold off merging until after that PR has gone in. Also happy to head in a different direction if you have any other ideas. LMK what you think.

import { quickPickValue } from '../../vscode/quickPick'
import { ParamOrMetric } from '../webview/contract'

export const pickFromParamsAndMetrics = (
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.

@mattseddon mattseddon changed the base branch from master to rename-legacy-variables July 28, 2021 05:35
@mattseddon mattseddon requested a review from rogermparent July 28, 2021 05:52
@mattseddon mattseddon changed the base branch from rename-legacy-variables to master July 28, 2021 05:52
@mattseddon mattseddon changed the base branch from master to rename-legacy-variables July 28, 2021 05:53
@mattseddon mattseddon marked this pull request as ready for review July 28, 2021 05:53
@@ -504,17 +504,17 @@
"view/title": [
{
"command": "dvc.runExperiment",
"when": "view == dvc.views.experimentsRunsTree",
"when": "view == dvc.views.experimentsTree",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[F] With the new placement this name actually makes sense now

@@ -0,0 +1,189 @@
import { join } from 'path'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[F] These tests are just lifted from the original quickPick test file

Base automatically changed from rename-legacy-variables to master July 28, 2021 18:41
@rogermparent rogermparent enabled auto-merge (squash) July 28, 2021 18:44
@codeclimate
Copy link

codeclimate bot commented Jul 28, 2021

Code Climate has analyzed commit 4118eda and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 6

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 94.1% (0.0% change).

View more on Code Climate.

@rogermparent rogermparent merged commit 49cff64 into master Jul 28, 2021
@rogermparent rogermparent deleted the group-files-around-concepts branch July 28, 2021 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants