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

Use Configy to read configuration, let env override config #545

Merged
merged 2 commits into from
Jun 30, 2023

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Jun 25, 2023

Normally, the environment variables always take preference on the configuration.
Hence, changing the priority here makes sense. Additionally, Dub already vendors
Configy, which we can just use instead of manual parsing.

Normally, the environment variables always take preference on the configuration.
Hence, changing the priority here makes sense. Additionally, Dub already vendors
Configy, which we can just use instead of manual parsing.
@s-ludwig
Copy link
Member

Is the @Name something provided by Configy? Or more importantly, does it expect the camelCase names in the JSON file, or the @Name ones? It's not really visible from the source code now, so at least a comment, or better yet a test, is probably a good idea here.

@Geod24
Copy link
Member Author

Geod24 commented Jun 27, 2023

Is the @Name something provided by Configy? Or more importantly, does it expect the camelCase names in the JSON file, or the @Name ones? It's not really visible from the source code now, so at least a comment, or better yet a test, is probably a good idea here.

It expects the Name one. Name and Optional are in the attributes module.
Moved the code to a new module (since dub test ignores unittests in the app module) and added two tests covering hopefully all features.

@s-ludwig
Copy link
Member

Okay, I see I got the previous behavior backwards, so this is good!

@Geod24 Geod24 merged commit 27e4589 into dlang:master Jun 30, 2023
2 checks passed
@Geod24 Geod24 deleted the configy branch June 30, 2023 09:54
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