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

Have FileSystemError adopt ErrorNoTelemetry #155885

Merged
merged 4 commits into from Aug 16, 2022
Merged

Conversation

lramos15
Copy link
Member

Fixes #155492

Allow setting error name with ErrorNoTelemetry. Tested on throwing a filesystem error but it presents a modal without any notion of ErrorNoTelemetry. If anyone has a notifcation example I can also test making sure it doesn't show up there.

@lramos15 lramos15 enabled auto-merge (squash) July 21, 2022 19:07
@lramos15 lramos15 self-assigned this Jul 21, 2022
@lramos15 lramos15 requested review from bpasero and sbatten July 21, 2022 19:07
@lramos15 lramos15 modified the milestones: August 2022, July 2022 Jul 21, 2022
@sbatten
Copy link
Member

sbatten commented Jul 21, 2022

@lramos15 does it have ENOPRO in the dialog?

@lramos15
Copy link
Member Author

@lramos15 does it have ENOPRO in the dialog?

I didn't test ENOPRO specifically I tested the error listed in #155492 but the modal just had the message and not the name

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

This may be user facing in certain cases, so what is the reason for making the error name rather cryptic by concatenating ErrorNoTelemetry and the original name? I would rather use something that is more generic, since I believe the main intent is to implement isErrorNoTelemetry properly:

public override set name(newName: string) {
	this._name = `${newName} (CodeExpectedError)`;
}

public static isErrorNoTelemetry(err: Error): err is ErrorNoTelemetry {
	return err.name.contains('(CodeExpectedError)');
}

I think CodeExpectedError can be something else too, if we want to go with node.js notion, maybe EEXPECTED).

src/vs/base/common/errors.ts Show resolved Hide resolved
src/vs/base/common/errors.ts Outdated Show resolved Hide resolved
@lramos15
Copy link
Member Author

This may be user facing in certain cases, so what is the reason for making the error name rather cryptic by concatenating ErrorNoTelemetry and the original name? I would rather use something that is more generic, since I believe the main intent is to implement isErrorNoTelemetry properly:

Yeah we can go with anything here. I think the main goal with sticking with ErrorNoTelemetry is to also try and clean it wherever it shows up

@lramos15 lramos15 modified the milestones: July 2022, August 2022 Jul 25, 2022
@lramos15 lramos15 assigned lramos15 and unassigned lramos15 Jul 26, 2022
@lramos15 lramos15 modified the milestones: August 2022, On Deck Aug 16, 2022
@lramos15 lramos15 requested a review from bpasero August 16, 2022 12:56
@lramos15 lramos15 modified the milestones: On Deck, August 2022 Aug 16, 2022
@lramos15
Copy link
Member Author

I went with a very simple solution here as further exploration looked like it was going to break things down the line. I believe FileSystemErrors are the most complex case so my fix is ok but I wish everything could go through ErrorNoTelemetry to make it clear. Will reinvestigate what we can do to unblock the custom error properties in the future but for now this helps cleanup our data pipeline by removing file system errors

@lramos15 lramos15 merged commit f152032 into main Aug 16, 2022
@lramos15 lramos15 deleted the lramos15/absolute-cattle branch August 16, 2022 13:35
@github-actions github-actions bot locked and limited conversation to collaborators Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to read file errors are being localized and showing up in telemetry
3 participants