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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethrow the compilation error after printing the message. #315

Merged
merged 4 commits into from Nov 22, 2016

Conversation

fatfisz
Copy link
Contributor

@fatfisz fatfisz commented Jun 8, 2016

Fixes #314

I wanted to sign the CLA, but I don't know what would The Dojo Foundation need my address for 馃槙 Is there any other way in which we can move forward?

@XhmikosR
Copy link
Member

Shouldn't we use grunt.log instead of throw?

Also, can you add a test case?

@fatfisz
Copy link
Contributor Author

fatfisz commented Jul 31, 2016

Sorry, forgot to respond.

If we only grunt.log error, the compiled.push(output.css); line will still throw. I think that rethrowing in this case and passing the error to the rejection handler (function(err) { nextFileObj(err); }) is the best option.

About the logging thing, doesn't less itself log something? I don't remember rn, will check tomorrow.

About the test case: I will add it soon.

@fatfisz
Copy link
Contributor Author

fatfisz commented Jul 31, 2016

I've checked what's happening with the logs and lessError(err, srcFile) already takes care of that.

So the error is logged, the problem is with the swallowed exception.

@fatfisz
Copy link
Contributor Author

fatfisz commented Jul 31, 2016

I've added a test, but it requires a bit of an explanation, I guess.

Because the whole task code is kept in one file, unit testing is impossible. Therefore for the test I've decided to rely on the fail.report Grunt method that prints either Done, but with warnings. or Done.. When control is not returned to Grunt, this method is not called and so the Done word is never printed (which is what happens now).

Also the --force flag needs to be passed in the test because otherwise grunt.fail.warn in lessError will cause an abrupt stop and make testing impossible.

@XhmikosR
Copy link
Member

XhmikosR commented Aug 2, 2016

Can't you just specify force for this one specific task in Gruntfile?

@fatfisz
Copy link
Contributor Author

fatfisz commented Aug 2, 2016

I don't understand? It's already specified in the test.

@XhmikosR
Copy link
Member

XhmikosR commented Aug 2, 2016

Without going to all this trouble and doing all kind of hacks like renaming the task, which is something I certainly don't like myself.

@fatfisz
Copy link
Contributor Author

fatfisz commented Aug 3, 2016

Oh, about that - I wanted to exclude the erroring task from the rest of the batch. If you are not convinced, I could use the force indeed.

@XhmikosR
Copy link
Member

XhmikosR commented Aug 3, 2016

I think it will be cleaner; just force that one specific task.

@fatfisz
Copy link
Contributor Author

fatfisz commented Aug 3, 2016

Ok, will do that in a few hours or so (I'm still at work).

@fatfisz
Copy link
Contributor Author

fatfisz commented Aug 3, 2016

I got lost; how can I force only one task? gruntjs/grunt#810 is still open, only workarounds exist and not a built-in solution. Wat do?

@XhmikosR
Copy link
Member

XhmikosR commented Aug 9, 2016

@fatfisz: did you try passing force: true in the specific subtask's options?

@fatfisz
Copy link
Contributor Author

fatfisz commented Aug 10, 2016

Tried it, not working.
image

@fatfisz
Copy link
Contributor Author

fatfisz commented Aug 10, 2016

(also tried with passing force: true not in the options property, but directly, with the same result)

@XhmikosR XhmikosR merged commit a507646 into gruntjs:master Nov 22, 2016
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