Add friendly error message for missing dotfiles #555
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I signed it! |
|
CLAs look good, thanks! |
|
|
||
| var checked = 0; | ||
| necessaryFiles.forEach(function(file) { | ||
| fs.stat(__dirname + '/' + file, function(err, stats) { |
|
@samccone Is this ok? |
| var filesMissing = []; | ||
|
|
||
| var checked = 0; | ||
| necessaryFiles.forEach(function(file) { |
There was a problem hiding this comment.
can we do
necessaryFiles.map(function(v){ return path.join(__dirname, v); })
reduce or filter down
then do the error check?
(I think in this case it is totally ok to use statSync just to keep the complexity low)
|
@ohanhi just ping me if you have any questions or whatever, or let me know any thoughts thanks again for this great PR! |
|
I'd like to offer a dissenting opinion here. The gulpfile for this project is already extremely complicated and due for some house cleaning. I would vote against adding another task to check for missing files. We have a record of the problem and the fix documented in #544. |
|
The flip side is that this maks an error (which several people now have hit) into a human readable easy to debug locally issue as compared to filing an issue on the bug tracker. For that reason I am 👍 to it. Most people never touch the gulp file, and those that do are applying recipes.. which cause issues for other reasons .. but we have an idea how to fix that set of issues #552 so, +1 to better errors --- the complexity argument is valid however I think the only reason why I see it as an issue are for the reasons stated in 552 |
|
I think this can be improved with a few lines of instructions in the README vs adding code to the project to support it. |
| // Check the files necessary for linting exist. | ||
| // Without this, the 'lint' task can fail with a very non-descriptive message: | ||
| // "TypeError: Cannot convert undefined or null to object" | ||
| gulp.task('lintfilesExist', function(cb) { |
There was a problem hiding this comment.
Instead of creating a separate task for this, could we move the code inside the lint task itself? @samccone wdyt?
There was a problem hiding this comment.
I did a variation on this which seems to work ok: https://gist.github.com/robdodson/9219e1b5042eeca14923
I'm not an expert in throwing errors in Gulp but here's what the output looks like:
There was a problem hiding this comment.
personally I think a separate task is pretty nice, easier to test... debug ... ect ect
|
moved to #580 |
Fixes #544.
This provides much more human-readable error messages for when certain dotfiles (namely
.jscsrcor.jshintrc) are missing from the file system.With the improvement:
Previously:
