Allow config file to be arbitrarily named#66
Conversation
bin/suitcss
Outdated
| var input = program.args[0] ? resolve(program.args[0]) : null; | ||
| var output = program.args[1] ? resolve(program.args[1]) : null; | ||
| var config = program.config ? require(resolve(program.config)) : null; | ||
| var config = null; |
There was a problem hiding this comment.
I guess that this could just be declared var config;
There was a problem hiding this comment.
hm, now this makes node run lint bark:
suit-preprocessor/bin/suitcss
94:1 error Combine this with the previous 'var' statement with uninitialized variables one-var
I guess lint suggests to read sth like
var config, currentWatchedFiles;
...but since the vars are rather unrelated in their sections, should we really?!
bin/suitcss
Outdated
| if (configFile && configFile.endsWith('.js')) { | ||
| config = require(resolve(configFile)); | ||
| } else if (configFile) { | ||
| config = fs.readJsonSync(configFile); |
There was a problem hiding this comment.
ouch, absolutely! : )
giuseppeg
left a comment
giuseppeg
left a comment
There was a problem hiding this comment.
Please fix the eslint failure either by combining this declaration with the previous one or reverting the change and setting it to null and then this is good to go!
bin/suitcss
Outdated
| config = require(resolve(configFile)); | ||
| } else if (configFile) { | ||
| config = fs.readJsonSync(resolve(configFile)); | ||
| } |
There was a problem hiding this comment.
Maybe we could put this into a function to reduce the duplication? Something like:
var config = resolveConfig(program.config);
function resolveConfig(configFile) {
if (!configFile) {return;}
var configPath = resolve(configFile);
var ext = path.extname(configPath);
if (/js|json/.test(ext)) {
return require(configPath);
}
return fs.readJsonSync(configPath);
}Note I haven't tested the above, but it will allow us to leverage require for JSON files too and fix the ESLint issue.
There was a problem hiding this comment.
sure, functions would also cleanup the cli code a bit - would be helpful also in the importRoot fix, btw.
I wasnt sure, if you guys wanted this inside the cli code. should these be factored out to something like bin/utils.js?
There was a problem hiding this comment.
My vote would be just add small functions in the bin file. I'd only split them out if we need them elsewhere. We can use rewire to unit test them.
There was a problem hiding this comment.
ah, I went too fast & pushed an factor out commit already, sry.
however, maybe once the importRoot stuff also gets more code in the cli, it might get a bit cluttered?
anyway, just ping me if you'd like this changed!
There was a problem hiding this comment.
My thoughts are unless we expect that utils file to be large and used in multiple places it just adds more misdirection to reading the code.
There was a problem hiding this comment.
@simonsmith alright, I factored the fn into bin/suitcss again.
also I left out unit tests for now, as the fns scope is covered by the integration tests already. good?
bin/utils.js
Outdated
| var configPath = resolve(configFile); | ||
| var ext = path.extname(configPath); | ||
|
|
||
| if (/js/.test(ext)) { |
There was a problem hiding this comment.
nitpick: we could avoid the regexp here with ext.indexOf('js') === 0 otherwise it should be ^js(?:on)?$
There was a problem hiding this comment.
hm, how about straight ext === '.js', then? : )
bin/suitcss
Outdated
| function requireOrParse(configFile) { | ||
| var configPath = resolve(configFile); | ||
| var ext = path.extname(configPath); | ||
| var readFn = ext === '.js' ? require : fs.readJsonSync; |
There was a problem hiding this comment.
if @simonsmith thinks that it is not a big deal to require .json files with readJsonSync (it could be required) then we can merge this.
There was a problem hiding this comment.
I think we should lean on require where possible. It seems wasteful to send JSON files through this extra module which basically does what require would already.
Could we reinstate the regex or check for json as well?
There was a problem hiding this comment.
imho it shows the purpose of the fn more clearly and since the dep on fs-extra is used elsewhere I thought 'hey, exactly what we want here' : )
but anyway: your call, I'll push an update in a minute!
|
Just re-running the windows build |
|
I think we could squash this into one commit and just reference the issue in the description. Are you happy to do that @casio? |
casio
force-pushed
the
feature/arbitrary-config-filename
branch
from
a189d3d to
69b73c1
Compare
|
@simonsmith yup, here you go |
|
cool thanks @casio! |
|
CLI forever. Thanks for this @casio 🎉 |
haha :D pleasure! |
3 participants
this is a stab at #65 - if you guys should choose to change the behaviour