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

Handle sub errors (base field is defined here, defined in this class) #45

Merged
merged 6 commits into from
Jul 3, 2018

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented Jun 30, 2018

Groups "Base field is defined here" and "Defined in this class" errors with their parent error.
Edit: also groups other "multi-error" errors like overload resolution failures, unification failures for fields from interfaces, etc.

Has no effect when not using friendly-errors-webpack-plugin.

Before:
2018-07-01-13 41-50-504385944

After:
2018-07-01-13 47-18-560434158

Overload resolution failure example:
before / after

Note: errors were already grouped by location, so the overload resolution failure was already short but not easy to read.

@kLabz
Copy link
Contributor Author

kLabz commented Jun 30, 2018

Whoops, typo. Do not merge yet.

@kLabz
Copy link
Contributor Author

kLabz commented Jun 30, 2018

Corrected + added more sub errors

@kLabz
Copy link
Contributor Author

kLabz commented Jul 1, 2018

Found a lot more errors to handle, working on it.

@kLabz
Copy link
Contributor Author

kLabz commented Jul 1, 2018

Should be ready, I did not find any other sub errrors in haxe compiler sources (there could be more that I missed, with a different code style, but they could be handled later if needed).

Seems tedious to add real-world tests for that: minimal sources to produce the haxe errors, get haxe's stderr when compiling them, extracts webpack errors from that, call errorFormatter.format() and check the result (or maybe just the resulting errors array). I don't have the courage to do that atm.

if (!previous || !previous.message) return false;
if (error.message === 'Defined in this class') return true;

if (previous.message === 'Could not find a suitable overload, reasons follow') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally that seem quite risky to rely on exact wording - now that said, if we go this route, it looks like error.message is usually clearly enough a sub error, even without checking previous.message (excepted that it is defined).

Copy link
Contributor Author

@kLabz kLabz Jul 1, 2018

Choose a reason for hiding this comment

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

I used exact wording only when the error messages are hardcoded in the compiler, and followed the rules there for sub errors.

I'd prefer some errors not to be grouped any more after a change in the compiler than errors being grouped when they are not related (could happen when a macro generates an error with a message similar to one of the "multiple-errors" generated by the compiler).

Copy link
Collaborator

@elsassph elsassph left a comment

Choose a reason for hiding this comment

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

Looks good

@elsassph elsassph merged commit abd198c into jasononeil:master Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants