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

[Bugfix] Parse on errorMatcher.set / handle no errorMatcher correctly #145

Merged
merged 6 commits into from
Aug 15, 2015

Conversation

MackieLoeffel
Copy link
Contributor

This PR fixes two bugs:

  • Errors are now highlighted, when the build is finished, not when the user goes to the first error
  • no error matching is performed, when no errorMatcher is set

@noseglid
Copy link
Owner

I tried to rebase this and run the test but I can't get it to work. Do you have time to look into this?

@MackieLoeffel
Copy link
Contributor Author

Done.
I also noticed, that the specs fail in many ways on windows, even if I start atom from cygwin. If I have time, I may look into fixing this.

};

ErrorMatcher.prototype._goto = function (id) {
var match = _.findWhere(this.currentMatch, { id: id });
if (!match) {
throw new Error('Can\'t find match with id ' + id);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't seem like this can ever occur. In the _gotoNext function it is protected by a check for 0 === this.currentMatch.length, and in the matchFirst function it is checked that there actually is a first match.
(If I missed some case where it actually happens, it should emit the error rather than throwing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct, that it schouldn't happen. It's more like defensive programming, if the other functions change in the future. But you are also correct, that it should emit an error.

noseglid added a commit that referenced this pull request Aug 15, 2015
[Bugfix] Parse on errorMatcher.set / handle no errorMatcher correctly
@noseglid noseglid merged commit 1c20f28 into noseglid:master Aug 15, 2015
@noseglid
Copy link
Owner

Thank you so much for this! Once again, sorry for taking so long to review this

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.

2 participants