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

Read source-dirs from elm.json #109

Closed
wants to merge 5 commits into from
Closed

Read source-dirs from elm.json #109

wants to merge 5 commits into from

Conversation

stoeffel
Copy link
Owner

@stoeffel stoeffel commented Jan 8, 2024

Respect source dirs in elm.json

@stoeffel stoeffel force-pushed the elm.json branch 2 times, most recently from 44787e7 to a11f8c4 Compare January 8, 2024 08:53
@stoeffel
Copy link
Owner Author

stoeffel commented Jan 8, 2024

cc @knuton

Copy link
Collaborator

@knuton knuton left a 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

bin/cli-helpers.js Outdated Show resolved Hide resolved
bin/cli-helpers.js Outdated Show resolved Hide resolved
Comment on lines +238 to +240
if (fs.existsSync(module)) {
return module;
}
Copy link
Collaborator

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).

Copy link
Owner Author

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>
CHANGELOG.md Outdated Show resolved Hide resolved
bin/runner.js Show resolved Hide resolved
stoeffel and others added 2 commits January 8, 2024 13:50
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(
Copy link
Collaborator

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

Copy link
Owner Author

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.

Copy link
Collaborator

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

okay removed completely.

Copy link
Collaborator

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.

@stoeffel stoeffel mentioned this pull request Jan 11, 2024
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.

3 participants