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

Improve tracked tree open file error notifications #528

Merged
merged 13 commits into from
Jun 5, 2021

Conversation

mattseddon
Copy link
Member

@mattseddon mattseddon commented Jun 4, 2021

From #330 (comment) - The level of the message is too high and there should be different actions based on whether the file is disk or appears binary.

This PR

  1. Downgrades the toast popup from an error to an information message.
  2. Adds an option to never see the information again if the file "appears to be binary".
  3. Offers a more useful error for when the file is not on disk.
  4. Gives the user the option to pull the file if it is missing.
  5. Gives the user the options to never see the information again if the file is missing.

Demo:

Screen.Recording.2021-06-04.at.5.01.10.pm.mov

LMK if you have any questions, comments, concerns.

Thanks,

@mattseddon mattseddon added the product PR that affects product label Jun 4, 2021
@mattseddon mattseddon self-assigned this Jun 4, 2021
@mattseddon mattseddon changed the title Improve tracked tree open file errors popup Improve tracked tree open file errors toast Jun 4, 2021
@mattseddon mattseddon changed the title Improve tracked tree open file errors toast Improve tracked tree open file errors Jun 4, 2021
@mattseddon mattseddon changed the title Improve tracked tree open file errors Improve tracked tree open file error notifications Jun 4, 2021

if (
error.message.includes(
'File seems to be binary and cannot be opened as text'
Copy link
Member Author

@mattseddon mattseddon Jun 4, 2021

Choose a reason for hiding this comment

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

[F] There is nothing in the error object outside of the message that is useful in terms of deciphering what the error was. Relying on "free text" is risky and brittle but there is no other option outside of removing openResource altogether.

it('should call showErrorMessage when dvc.views.trackedExplorerTree.openFile tries to open a binary file', async () => {
const path = join(dvcDemoPath, 'model.pt')
const uri = Uri.file(path)
it('should only call showInformationMessage when trying to open a binary file without the no binary errors option set', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] This is when given, when, then would be super handy

@mattseddon mattseddon marked this pull request as ready for review June 4, 2021 05:39
@mattseddon mattseddon force-pushed the improve-open-resource-errors branch from 698c6e4 to 979f707 Compare June 4, 2021 07:25
@mattseddon mattseddon enabled auto-merge June 5, 2021 07:38
@codeclimate
Copy link

codeclimate bot commented Jun 5, 2021

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

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

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

View more on Code Climate.

@mattseddon mattseddon merged commit e317a98 into master Jun 5, 2021
@mattseddon mattseddon deleted the improve-open-resource-errors branch June 5, 2021 07:43
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