Skip to content

Conversation

@BatuhanSA
Copy link
Contributor

Implemented a function that hierarchically loads configuration keys and values from JSON config files and CLI arguments, with an option to specify the target file name.

Copy link
Contributor

@eriq-augustine eriq-augustine left a comment

Choose a reason for hiding this comment

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

Be careful, there are several mistakes here that should not be.

We will also want to get the CLI with this.
We can break this up into two PRs:

  • This one.
  • One that adds the CLI.

Copy link
Contributor

@eriq-augustine eriq-augustine left a comment

Choose a reason for hiding this comment

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

Tests are generally looking good.

Copy link
Contributor

@eriq-augustine eriq-augustine left a comment

Choose a reason for hiding this comment

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

Looking pretty good.

We also need some text in the README covering how config works.

Copy link
Contributor

@eriq-augustine eriq-augustine left a comment

Choose a reason for hiding this comment

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

Nice!
Looking pretty good.

Copy link
Contributor

@eriq-augustine eriq-augustine left a comment

Choose a reason for hiding this comment

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

Documentation is looking much better.

I did not fully comment the new CLI option table,
since it needs some big changes.

Copy link
Contributor

@eriq-augustine eriq-augustine left a comment

Choose a reason for hiding this comment

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

Documentation is getting pretty close.

Copy link
Contributor

@eriq-augustine eriq-augustine left a comment

Choose a reason for hiding this comment

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

Getting better.
The source intros really help.

Copy link
Contributor

@eriq-augustine eriq-augustine left a comment

Choose a reason for hiding this comment

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

lgtm

@eriq-augustine eriq-augustine merged commit 2555b73 into edulinq:main Sep 5, 2025
43 checks passed
@BatuhanSA BatuhanSA deleted the config branch September 6, 2025 00:39
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.

2 participants