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

Simplify DVC CLI Location/Version Logic #3784

Merged
merged 11 commits into from
May 3, 2023
Merged

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Apr 28, 2023

  • always opens "Setup" if extension status button is clicked on
  • simplifies Toasts text when DVC CLI version is not found or incorrect (details in comment)

Demo

Note: Toast and extension button are hard to see in the demo since they're at the bottom of the screen 😅

https://user-images.githubusercontent.com/43496356/235555040-3615e203-d6df-45ef-b9f5-630a589b183e.mov

https://user-images.githubusercontent.com/43496356/235725878-10c11d86-e6ed-439d-8102-c4c38523a4e5.mov

Screen.Recording.2023-05-03.at.9.18.38.AM.mov

Part of #3434

@julieg18 julieg18 added the product PR that affects product label Apr 28, 2023
@julieg18 julieg18 self-assigned this Apr 28, 2023
import { getFirstWorkspaceFolder } from '../../vscode/workspaceFolders'
import { delay } from '../../util/time'
import { SetupSection } from '../../setup/webview/contract'

const warnWithSetupAction = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current possible options are:

Version not found:
An error was thrown when trying to access the CLI.
buttons: Setup, Don't Show Again

Version can not be verified:
The extension cannot initialize as we were unable to verify the DVC CLI version.
Button: Setup

Version is found but it's not up to date:
The extension cannot initialize because you are using the wrong version of the CLI
Buttons: Setup

Version is found but it's too far ahead:
The extension cannot initialize because you are using the wrong version of the extension.
Button: Setup

What do we think? Should the toasts be more/less simplified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated Toasts:

Version not found:
An error was thrown when trying to access the CLI.
buttons: Setup, Don't Show Again

Version can not be verified:
The extension cannot initialize as we were unable to verify the DVC CLI version.
Button: Setup

Version behind or ahead
The extension cannot initialize because the DVC CLI version is incompatible.
Button: Setup

extension/src/status.ts Outdated Show resolved Hide resolved
@julieg18 julieg18 changed the title Simplify DVC Cli Toast messages Simplify DVC CLI Location/Version Logic May 2, 2023
@julieg18 julieg18 force-pushed the clean-up-dvc-cli-toasts branch from e05c8f4 to 4b1f4a7 Compare May 2, 2023 16:14
)
}

export const warnAheadOfLatestTested = (): void => {
const warnAheadOfLatestTested = (): void => {
void Toast.warnWithOptions(
`The located DVC CLI is at least a minor version ahead of the latest version the extension was tested with (${LATEST_TESTED_CLI_VERSION}). This could lead to unexpected behaviour. Please upgrade to the most recent version of the extension and reload this window.`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left this toast alone for now since we don't have any info on the latest tested cli version in Setup yet. Once we update our DVC Setup page (task is in #3434 list), we can simplify this one or remove entirely.

Copy link
Member

Choose a reason for hiding this comment

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

let's aim to remove it

extension/src/status.ts Show resolved Hide resolved
@julieg18 julieg18 marked this pull request as ready for review May 2, 2023 16:56
@julieg18 julieg18 requested a review from sroy3 as a code owner May 2, 2023 16:56
@julieg18 julieg18 requested review from shcheklein and mattseddon May 2, 2023 16:57
extension/src/cli/dvc/discovery.ts Outdated Show resolved Hide resolved
)
}

export const warnAheadOfLatestTested = (): void => {
const warnAheadOfLatestTested = (): void => {
void Toast.warnWithOptions(
`The located DVC CLI is at least a minor version ahead of the latest version the extension was tested with (${LATEST_TESTED_CLI_VERSION}). This could lead to unexpected behaviour. Please upgrade to the most recent version of the extension and reload this window.`
Copy link
Member

Choose a reason for hiding this comment

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

let's aim to remove it

@codeclimate
Copy link

codeclimate bot commented May 3, 2023

Code Climate has analyzed commit b7dcf84 and detected 0 issues on this pull request.

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

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

View more on Code Climate.

@julieg18 julieg18 enabled auto-merge (squash) May 3, 2023 14:34
@julieg18 julieg18 merged commit 37478d8 into main May 3, 2023
@julieg18 julieg18 deleted the clean-up-dvc-cli-toasts branch May 3, 2023 14:38
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.

2 participants