-
Notifications
You must be signed in to change notification settings - Fork 14
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
Read source-dirs from elm.json #109
Conversation
44787e7
to
a11f8c4
Compare
cc @knuton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I think this might be a chance to introduce a convention for making configuring the elm root optional.
Noting that this PR would close #67
if (fs.existsSync(module)) { | ||
return module; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optionally: Require exactly one match, failing explicitly if module reference is ambiguous (> 1 matches).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think elm make fails if there are more than one modules with the same name.
Co-authored-by: Johannes Emerich <johannes@emerich.de>
Co-authored-by: Jakub Hampl <kopomir@gmail.com>
verifyExamples["elm-root"], | ||
"elm.json" | ||
); | ||
elmJson = require(elmJsonPath); | ||
} catch (e) { | ||
console.log(`Copying initial elm-verify-examples.json to ${configPath}`); | ||
fsExtra.copySync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So perhaps here we can now just remove this bit, and instead just
var elmJsonPath = path.join(
path.dirname(configPath),
"..",
"elm.json"
);
elmJson = require(elmJsonPath);
verifyExamples = {};
}
or some such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I thought we just make it optional not remove it completely, but maybe removing it is fine as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By optional I mean that the file doesn't have to exist - if it doesn't you get sensible defaults that should work for most projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay removed completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're slightly talk past each other. What I've been proposing is:
Make the existence of elm-verify-examples.json
file optional. If it is missing, the default place we find elmRoot is in the containing folder. The default for which modules to run is the exposed ones plus Readme.md.
This change instead removes the ability to configure the elmRoot
. I think that's also interesting, although probably to maintain some flexibility, I'd suggest searching containing folders recursively.
Respect source dirs in elm.json