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

Merge 10-tomlparams-supporting-defaults-as-directory-of-toml-files -> main #11

Merged

Conversation

github-actions[bot]
Copy link

Automated changes by create-pull-request GitHub action

@github-actions github-actions bot linked an issue Aug 30, 2023 that may be closed by this pull request
Copy link
Contributor

@derekmiddlemiss derekmiddlemiss 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, few minor comments!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tidy-ups in here, cheers!

params_default_as_file[default_primary_key].get_params()
)
)
self.assertEqual(d_as_dir, d_as_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will order of keys matter here too in this comparison? If so, would we want the test to fail if key order differed? If not, might be worth comparing sets after concatenate_keys() here?

@@ -34,3 +34,23 @@ def load_toml(path):
f'\n***\n*** Problem parsing {path}:\n***\n', file=sys.stderr
)
raise


def concatenate_keys(d: dict, sep='.'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Mark return type here? Think it would be Generator[tuple[str, Any], None, None]. Or could be more specific and replace Any with supported types in TOML.

Hmm, actually - do we know that the keys will always be strings? Is that a TOML requirement?

' is not allow in the consolidated defaults.'
)
if defaults:
defaults_concatenated_keys = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make defaults_concatenated_keys and toml_dict_concatenated_keys here sets, and you could then do

if keys_in_common := defaults_concatenated_keys.intersection(toml_dict_concatenated_keys):
    raise KeyError(
                            f"Duplicated key(s) '{keys_in_common}' in {toml}. Check"
                            f" any of the files in {all_tomls[:i]}"
                        )

- Add corrrect return type in concatenate_keys
- test_content_defaults_as_directory tuple -> set
- tomlparams.py: defaults_concatenated_keys -> set
- Remove test option from the cli
- test if we have repeated keys in one of the toml to composed
defaults aas directory.
Copy link
Contributor

@derekmiddlemiss derekmiddlemiss 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 - one minor comment as to whether we need all the fish, animals, fungi tomls again for the repeated keys test?

@@ -0,0 +1,2 @@
[human]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the repeated key right? Maybe time down the number of other files for this one test, as that behaviour already well tested previously

- we creatse as_dict method on params_group to use for hashing.
@vadrienzo vadrienzo merged commit 5bbdf6c into main Aug 31, 2023
4 checks passed
@vadrienzo vadrienzo deleted the 10-tomlparams-supporting-defaults-as-directory-of-toml-files branch August 31, 2023 17:43
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.

tomlparams-supporting-defaults-as-directory-of-TOML-files
2 participants