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

Error auto parse #90

Merged
merged 7 commits into from
Jun 7, 2015
Merged

Error auto parse #90

merged 7 commits into from
Jun 7, 2015

Conversation

5n8ke
Copy link
Contributor

@5n8ke 5n8ke commented Jun 4, 2015

Added config to automatically parse and scroll to the first matched error after a build fails.

@noseglid
Copy link
Owner

noseglid commented Jun 4, 2015

This looks good! Could you also add a test for this?

@5n8ke
Copy link
Contributor Author

5n8ke commented Jun 6, 2015

I think the test should be fine this way.
I am not pleased with the fact, that the config is not part of the errorMatcher module, but I don't think there is an easy way to move it out of the main package.


atom.config.set('build.scrollOnError', true);
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably not be here.
You should also set it to the default value in the beforeEach (which is false in this case).

@noseglid
Copy link
Owner

noseglid commented Jun 6, 2015

Excellent. Just a minor review comment then we're good to merge!

@5n8ke
Copy link
Contributor Author

5n8ke commented Jun 6, 2015

Fixed my copy and paste error and added the default config in the beforeEach.

Edit:
But I'm still new to this whole test system and there are some parts I don't seem to understand...
Edit2:
If the config.set in the actual test is moved inside the preceding runs() the error can be avoided. But I'm not sure whether this is good practice or not.


atom.config.set('build.scrollOnError', false);
Copy link
Owner

Choose a reason for hiding this comment

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

This line shouldn't be here at all.
Since waitsFor and runs are async, this line will reset the configuration to false before your test even runs.

After putting the configuration setting in beforeEach, we've made sure each test always start from the same fixture. There's no need to "reset" the configuration here.

@5n8ke
Copy link
Contributor Author

5n8ke commented Jun 6, 2015

Ah, now I understand. That is the problem of working at two in the night 😄

noseglid added a commit that referenced this pull request Jun 7, 2015
@noseglid noseglid merged commit ad657d1 into noseglid:master Jun 7, 2015
@noseglid
Copy link
Owner

noseglid commented Jun 7, 2015

Splendied! Thank you so much for 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