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

Use diff in repository class #451

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
a4380bd
switch modified to use diff
mattseddon May 20, 2021
8913790
remove unnecessary or
mattseddon May 20, 2021
73cbab3
switch deleted to use diff
mattseddon May 20, 2021
7c9de40
switch added to use diff
mattseddon May 20, 2021
a44824a
switch not in cache to use diff
mattseddon May 20, 2021
175a05d
remove status from tests
mattseddon May 20, 2021
5495340
dry up code
mattseddon May 20, 2021
9c7b748
remove usage of status from repo
mattseddon May 20, 2021
d484c58
stub diff in integration test
mattseddon May 20, 2021
cf73cd7
Merge branch 'switch-repo-to-diff' into remove-usage-of-status
mattseddon May 20, 2021
4b2cb4f
remove dead stub
mattseddon May 20, 2021
f876fbd
remove reader function
mattseddon May 20, 2021
efdd6ba
Merge branch 'master' into switch-repo-to-diff
mattseddon May 20, 2021
0091a63
Merge branch 'switch-repo-to-diff' of https://github.com/iterative/vs…
mattseddon May 20, 2021
11e4b31
reword test
mattseddon May 20, 2021
5bd372f
Merge branch 'master' of https://github.com/iterative/vscode-dvc into…
mattseddon May 20, 2021
dce84d8
replace text with variable
mattseddon May 20, 2021
0972448
handle case where diff is empty
mattseddon May 21, 2021
89bcef4
bring back status
mattseddon May 21, 2021
d881810
add stage modified status
mattseddon May 21, 2021
1e39136
remove filter from reduceStatuses
mattseddon May 21, 2021
d28af2c
reduce the number of return statements in provideFileDecoration
mattseddon May 21, 2021
8100fc6
Merge branch 'refactor-provide-file-decoration' into switch-repo-to-diff
mattseddon May 21, 2021
ac58ee8
try and account for difference between diff and status
mattseddon May 21, 2021
aeb71c8
test update
mattseddon May 21, 2021
4e8176d
Merge branch 'master' of https://github.com/iterative/vscode-dvc into…
mattseddon May 24, 2021
c6604ad
move handling of output into state class
mattseddon May 24, 2021
54bb410
move updateTracked into repository state
mattseddon May 24, 2021
d5465b3
move updateUntracked inside of repository state
mattseddon May 24, 2021
7f374d2
move state into own file
mattseddon May 24, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions extension/src/Repository/DecorationProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,20 @@ import { isStringInEnum } from '../util'
export type DecorationState = Record<Status, Set<string>>

enum Status {
ADDED = 'added',
DELETED = 'deleted',
MODIFIED = 'modified',
NEW = 'new',
NOT_IN_CACHE = 'notInCache',
TRACKED = 'tracked'
}

export class DecorationProvider implements FileDecorationProvider {
private static DecorationAdded: FileDecoration = {
badge: 'A',
color: new ThemeColor('gitDecoration.addedResourceForeground'),
tooltip: 'DVC added'
}

private static DecorationDeleted: FileDecoration = {
badge: 'D',
color: new ThemeColor('gitDecoration.deletedResourceForeground'),
Expand All @@ -34,12 +40,6 @@ export class DecorationProvider implements FileDecorationProvider {
tooltip: 'DVC modified'
}

private static DecorationNew: FileDecoration = {
badge: 'A',
color: new ThemeColor('gitDecoration.addedResourceForeground'),
tooltip: 'DVC added'
}

private static DecorationNotInCache: FileDecoration = {
badge: 'NC',
color: new ThemeColor('gitDecoration.renamedResourceForeground'),
Expand Down Expand Up @@ -102,8 +102,8 @@ export class DecorationProvider implements FileDecorationProvider {
if (this.state.deleted?.has(uri.fsPath)) {
return DecorationProvider.DecorationDeleted
}
if (this.state.new?.has(uri.fsPath)) {
return DecorationProvider.DecorationNew
if (this.state.added?.has(uri.fsPath)) {
return DecorationProvider.DecorationAdded
}
if (this.state.notInCache?.has(uri.fsPath)) {
return DecorationProvider.DecorationNotInCache
Expand Down
119 changes: 58 additions & 61 deletions extension/src/Repository/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@ import { Config } from '../Config'
import { SourceControlManagement } from './views/SourceControlManagement'
import { mocked } from 'ts-jest/utils'
import { DecorationProvider } from './DecorationProvider'
import { Repository, RepositoryState, Status } from '.'
import { listDvcOnlyRecursive, ListOutput, status } from '../cli/reader'
import { Repository, RepositoryState } from '.'
import {
diff,
DiffOutput,
listDvcOnlyRecursive,
ListOutput
} from '../cli/reader'
import { getAllUntracked } from '../git'

jest.mock('@hediet/std/disposable')
Expand All @@ -15,8 +20,8 @@ jest.mock('../cli/reader')
jest.mock('../git')
jest.mock('../fileSystem')

const mockedDiff = mocked(diff)
const mockedListDvcOnlyRecursive = mocked(listDvcOnlyRecursive)
const mockedStatus = mocked(status)
const mockedGetAllUntracked = mocked(getAllUntracked)

const mockedSourceControlManagement = mocked(SourceControlManagement)
Expand Down Expand Up @@ -52,6 +57,14 @@ beforeEach(() => {
describe('Repository', () => {
const dvcRoot = resolve(__dirname, '..', '..', 'demo')

const emptyDiff = {
added: [],
modified: [],
deleted: [],
renamed: [],
'not in cache': []
}

describe('ready', () => {
it('should wait for the state to be ready before resolving', async () => {
const logDir = 'logs'
Expand All @@ -67,16 +80,19 @@ describe('Repository', () => {
{ path: rawDataDir }
] as ListOutput[])

mockedStatus.mockResolvedValueOnce({
train: [
{ 'changed deps': { 'data/MNIST': 'modified' } },
{ 'changed outs': { 'model.pt': 'modified', logs: 'modified' } },
'always changed'
mockedDiff.mockResolvedValueOnce({
added: [],
deleted: [],
modified: [
{ path: 'model.pt' },
{ path: logDir },
{ path: logAcc },
{ path: logLoss },
{ path: MNISTDataDir }
],
'data/MNIST/raw.dvc': [
{ 'changed outs': { 'data/MNIST/raw': 'modified' } }
]
} as Record<string, (Record<string, Record<string, string>> | string)[]>)
'not in cache': [],
renamed: []
} as DiffOutput)

const untracked = new Set([
resolve(dvcRoot, 'some', 'untracked', 'python.py')
Expand All @@ -91,7 +107,13 @@ describe('Repository', () => {
const repository = new Repository(dvcRoot, config, decorationProvider)
await repository.isReady()

const modified = new Set([resolve(dvcRoot, rawDataDir)])
const modified = new Set([
resolve(dvcRoot, rawDataDir),
resolve(dvcRoot, logDir),
resolve(dvcRoot, logAcc),
resolve(dvcRoot, logLoss),
resolve(dvcRoot, model)
])
const tracked = new Set([
resolve(dvcRoot, logAcc),
resolve(dvcRoot, logLoss),
Expand All @@ -108,18 +130,18 @@ describe('Repository', () => {
pythonBinPath: undefined
}

expect(mockedStatus).toBeCalledWith(expectedExecutionOptions)
expect(mockedDiff).toBeCalledWith(expectedExecutionOptions)
expect(mockedGetAllUntracked).toBeCalledWith(dvcRoot)
expect(mockedListDvcOnlyRecursive).toBeCalledWith(
expectedExecutionOptions
)

expect(repository.getState()).toEqual(
expect.objectContaining({
added: emptySet,
dispose: Disposable.fn(),
deleted: emptySet,
notInCache: emptySet,
new: emptySet,
modified,
tracked,
untracked
Expand All @@ -131,7 +153,7 @@ describe('Repository', () => {
describe('resetState', () => {
it('will not exclude changed outs from stages that are always changed', async () => {
mockedListDvcOnlyRecursive.mockResolvedValueOnce([])
mockedStatus.mockResolvedValueOnce({})
mockedDiff.mockResolvedValueOnce(emptyDiff)
mockedGetAllUntracked.mockResolvedValueOnce(new Set())

const config = ({
Expand All @@ -150,18 +172,12 @@ describe('Repository', () => {
const logLoss = join(logDir, 'loss.tsv')
const model = 'model.pt'

mockedStatus.mockResolvedValueOnce({
train: [
{
'changed deps': { 'data/MNIST': 'modified', 'train.py': 'modified' }
},
{ 'changed outs': { 'model.pt': 'deleted' } },
'always changed'
],
'data/MNIST/raw.dvc': [
{ 'changed outs': { 'data/MNIST/raw': 'deleted' } }
]
} as Record<string, (Record<string, Record<string, string>> | string)[]>)
mockedDiff.mockResolvedValueOnce(({
added: [],
deleted: [{ path: model }, { path: dataDir }],
modified: [],
'not in cache': []
} as unknown) as DiffOutput)

const emptySet = new Set<string>()

Expand Down Expand Up @@ -197,15 +213,15 @@ describe('Repository', () => {
pythonBinPath: undefined
}

expect(mockedStatus).toBeCalledWith(expectedExecutionOptions)
expect(mockedDiff).toBeCalledWith(expectedExecutionOptions)
expect(mockedGetAllUntracked).toBeCalledWith(dvcRoot)
expect(mockedListDvcOnlyRecursive).toBeCalledWith(
expectedExecutionOptions
)

expect(repository.getState()).toEqual({
added: emptySet,
dispose: Disposable.fn(),
new: emptySet,
modified: emptySet,
notInCache: emptySet,
deleted,
Expand All @@ -216,7 +232,7 @@ describe('Repository', () => {

it("should update the classes state and call it's dependents", async () => {
mockedListDvcOnlyRecursive.mockResolvedValueOnce([])
mockedStatus.mockResolvedValueOnce({})
mockedDiff.mockResolvedValueOnce(emptyDiff)
mockedGetAllUntracked.mockResolvedValueOnce(new Set())

const config = ({
Expand All @@ -231,39 +247,20 @@ describe('Repository', () => {
const logAcc = join(logDir, 'acc.tsv')
const logLoss = join(logDir, 'loss.tsv')
const dataDir = 'data'
const model = 'model.pt'
const model = 'model.pkl'
mockedListDvcOnlyRecursive.mockResolvedValueOnce([
{ path: logAcc },
{ path: logLoss },
{ path: model },
{ path: dataDir }
] as ListOutput[])

mockedStatus.mockResolvedValueOnce({
prepare: [
{ 'changed deps': { 'data/data.xml': Status.NOT_IN_CACHE } },
{ 'changed outs': { 'data/prepared': Status.NOT_IN_CACHE } }
],
featurize: [
{ 'changed deps': { 'data/prepared': Status.NOT_IN_CACHE } },
{ 'changed outs': { 'data/features': 'modified' } }
],
train: [
{ 'changed deps': { 'data/features': 'modified' } },
{ 'changed outs': { 'model.pkl': 'deleted' } }
],
evaluate: [
{
'changed deps': {
'data/features': 'modified',
'model.pkl': 'deleted'
}
}
],
'data/data.xml.dvc': [
{ 'changed outs': { 'data/data.xml': Status.NOT_IN_CACHE } }
]
} as Record<string, (Record<string, Record<string, string>> | string)[]>)
mockedDiff.mockResolvedValueOnce(({
added: [],
modified: [{ path: 'data/features' }],
deleted: [{ path: model }],
'not in cache': [{ path: 'data/data.xml' }, { path: 'data/prepared' }]
} as unknown) as DiffOutput)

const untracked = new Set([
resolve(dvcRoot, 'some', 'untracked', 'python.py'),
Expand All @@ -279,8 +276,8 @@ describe('Repository', () => {
const deleted = new Set([join(dvcRoot, 'model.pkl')])
const modified = new Set([join(dvcRoot, 'data/features')])
const notInCache = new Set([
join(dvcRoot, 'data/data.xml'),
join(dvcRoot, 'data/prepared')
join(dvcRoot, 'data/prepared'),
join(dvcRoot, 'data/data.xml')
])
const tracked = new Set([
resolve(dvcRoot, logAcc),
Expand All @@ -296,15 +293,15 @@ describe('Repository', () => {
pythonBinPath: undefined
}

expect(mockedStatus).toBeCalledWith(expectedExecutionOptions)
expect(mockedDiff).toBeCalledWith(expectedExecutionOptions)
expect(mockedGetAllUntracked).toBeCalledWith(dvcRoot)
expect(mockedListDvcOnlyRecursive).toBeCalledWith(
expectedExecutionOptions
)

expect(repository.getState()).toEqual({
added: new Set(),
dispose: Disposable.fn(),
new: new Set(),
modified,
notInCache,
deleted,
Expand Down
36 changes: 23 additions & 13 deletions extension/src/Repository/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ import {
} from './views/SourceControlManagement'
import { DecorationProvider, DecorationState } from './DecorationProvider'
import { Deferred } from '@hediet/std/synchronization'
import { status, listDvcOnlyRecursive } from '../cli/reader'
import { diff, listDvcOnlyRecursive } from '../cli/reader'
import { dirname, join } from 'path'
import { observable, makeObservable } from 'mobx'
import { getExecutionOptions } from '../cli/execution'

export enum Status {
ADDED = 'added',
DELETED = 'deleted',
MODIFIED = 'modified',
NEW = 'new',
NOT_IN_CACHE = 'not in cache'
}

Expand All @@ -36,18 +36,18 @@ export class RepositoryState
implements DecorationState, SourceControlManagementState {
public dispose = Disposable.fn()

public tracked: Set<string>
public added: Set<string>
public deleted: Set<string>
public modified: Set<string>
public new: Set<string>
public notInCache: Set<string>
public tracked: Set<string>
public untracked: Set<string>

constructor() {
this.tracked = new Set<string>()
this.deleted = new Set<string>()
this.modified = new Set<string>()
this.new = new Set<string>()
this.added = new Set<string>()
this.notInCache = new Set<string>()
this.untracked = new Set<string>()
}
Expand Down Expand Up @@ -144,20 +144,30 @@ export class Repository {
return Object.values(filteredStatusOutput).reduce(statusReducer, {})
}

private async getStatus(): Promise<Partial<Record<Status, Set<string>>>> {
const options = getExecutionOptions(this.config, this.dvcRoot)
const statusOutput = (await status(options)) as StatusOutput
private getStatus(): Partial<Record<Status, Set<string>>> {
const statusOutput = {} as StatusOutput

return this.reduceToChangedOutsStatuses(statusOutput)
}

public async updateStatus() {
const status = await this.getStatus()
this.getStatus()
mattseddon marked this conversation as resolved.
Show resolved Hide resolved

const options = getExecutionOptions(this.config, this.dvcRoot)
const diffOutput = await diff(options)

this.state.modified = status.modified || new Set<string>()
this.state.deleted = status.deleted || new Set<string>()
this.state.new = status.new || new Set<string>()
this.state.notInCache = status['not in cache'] || new Set<string>()
this.state.added = new Set<string>(
diffOutput.added.map(entry => join(this.dvcRoot, entry.path))
)
this.state.modified = new Set<string>(
diffOutput.modified.map(entry => join(this.dvcRoot, entry.path))
)
this.state.deleted = new Set<string>(
diffOutput.deleted.map(entry => join(this.dvcRoot, entry.path))
)
this.state.notInCache = new Set<string>(
diffOutput['not in cache'].map(entry => join(this.dvcRoot, entry.path))
)
}

public async updateUntracked() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,24 +39,24 @@ describe('SourceControlManagement', () => {
expect(sourceControlManagement.getState()).toEqual([])

const updatedState = ({
added: new Set(['/some/new/path']),
deleted: new Set(['/some/deleted/path', '/some/other/deleted/path']),
dispose: () => undefined,
new: new Set(['/some/new/path']),
tracked: new Set(['/some/excluded/tracked/path'])
} as unknown) as SourceControlManagementState

sourceControlManagement.setState(updatedState)

expect(sourceControlManagement.getState()).toEqual([
{ resourceUri: Uri.file('/some/new/path'), contextValue: 'added' },
{
resourceUri: Uri.file('/some/deleted/path'),
contextValue: 'deleted'
},
{
resourceUri: Uri.file('/some/other/deleted/path'),
contextValue: 'deleted'
},
{ resourceUri: Uri.file('/some/new/path'), contextValue: 'new' }
}
])

sourceControlManagement.setState(initialState)
Expand Down
Loading