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

Skip early and log when prefix mismatches #96

Merged
merged 2 commits into from
Jan 16, 2017
Merged

Conversation

am11
Copy link
Collaborator

@am11 am11 commented Jan 15, 2017

  • If the requested file is not .css, log and skip (new: log, we were already skipping here)
  • If the requested file does not starts with provided prefix, log and prefix (new)
  • Provide reason for compile logs (new)
  • Add more logger tests

Fixes #88

@am11
Copy link
Collaborator Author

am11 commented Jan 15, 2017

@jacekkopecky, @silverlight513, can you please take a look?

@am11 am11 force-pushed the feature/gh-88-skip-logs branch from a2e59ec to 5454703 Compare January 15, 2017 18:47
if (debug) {
log('error', 'error', errorMessage || err);
}
log('error', 'error', errorMessage || err);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The severity level error should not be conditioned by debug check. Besides now the condition to check debug flag for severity debug in moved to the log function.

return compile();
}

// Already compiled, check imports
checkImports(sassPath, cssStats.mtime, function(changed) {
if (debug && changed && changed.length) {
changed.forEach(function(path) {
log('debug', 'modified import %s', path);
log('debug', 'compile', path, '(import file) was modified');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that %s here was not expanding, now the fourth parameter is interpolated in the flattened log message.

@@ -285,15 +298,15 @@ module.exports = function(options) {
}

if (sassStats.mtime > cssStats.mtime) { // Source has changed, compile it
if (debug) { log('debug', 'modified', cssPath); }
log('debug', 'compile', cssPath, 'was modified');
Copy link
Contributor

Choose a reason for hiding this comment

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

this implies the css was modified, not the scss; should the log have sassPath there instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this; this is fixed by bf8dfd9.

@jacekkopecky
Copy link
Contributor

This is very good, nicely consistent and much clearer, yes. Thanks for asking @am11

am11 added 2 commits January 16, 2017 01:35
* If the requested file is not .css, log and skip (new: log, we were already skipping here)
* If the requested file does not starts with provided prefix, log and prefix (new)
* Provide reason for compile logs (new)
* Add more logger tests

Fixes #88
@am11 am11 force-pushed the feature/gh-88-skip-logs branch from c95b745 to c3b12a2 Compare January 15, 2017 23:40
@am11
Copy link
Collaborator Author

am11 commented Jan 15, 2017

Thanks @jacekkopecky for the review and remarks, much appreciated. The feedback has been addressed.

@am11 am11 merged commit 3ecbe9f into master Jan 16, 2017
@am11 am11 deleted the feature/gh-88-skip-logs branch January 16, 2017 00:00
@am11
Copy link
Collaborator Author

am11 commented Jan 16, 2017

v0.11.0 has been released.

@jacekkopecky
Copy link
Contributor

Happy to help, cheers!

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.

prefix ignored when running
2 participants