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

feat: persist project config in deno.json #193

Merged
merged 13 commits into from
Nov 27, 2023
Merged

feat: persist project config in deno.json #193

merged 13 commits into from
Nov 27, 2023

Conversation

arnauorriols
Copy link
Member

No description provided.

Copy link
Member

@magurotuna magurotuna left a comment

Choose a reason for hiding this comment

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

My understanding is that it reads a config file and merges it to Args. But I'm a little bit concerned with this approach because Args is just a parsed result of std's parse function, which doesn't have any domain knowledge of individual applications.

What I'd like to propose is to define a new class/object that stores all configurable values (which I'll call Config) and construct Config from the config file, command line args, and even env vars, then we'll be able to use the Config instance throughout the process.

@arnauorriols
Copy link
Member Author

My understanding is that it reads a config file and merges it to Args. But I'm a little bit concerned with this approach because Args is just a parsed result of std's parse function, which doesn't have any domain knowledge of individual applications.

What I'd like to propose is to define a new class/object that stores all configurable values (which I'll call Config) and construct Config from the config file, command line args, and even env vars, then we'll be able to use the Config instance throughout the process.

I don't disagree, but I'd rather do it in a separate PR, as this work would be orthogonal to the scope of this PR. I've created the issue #196 for it

Copy link
Member

@magurotuna magurotuna left a comment

Choose a reason for hiding this comment

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

Question: will we add support for JSONC later?

@arnauorriols
Copy link
Member Author

Question: will we add support for JSONC later?

that's the plan, yes. I've created an issue also for this to keep track of it: #198

src/args.ts Outdated Show resolved Hide resolved
Copy link
Member

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

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

LGTM apart from the minor comments

@arnauorriols arnauorriols merged commit bd57e29 into main Nov 27, 2023
13 checks passed
@arnauorriols arnauorriols deleted the config-file branch November 27, 2023 11:30
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.

4 participants