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/write nested data as flattened parameter name to and from toml file #48

Merged
merged 13 commits into from
Mar 22, 2022

Conversation

sverhoeven
Copy link
Member

@sverhoeven sverhoeven commented Mar 1, 2022

Refs #47

The haddock3 catalogs have not been updated. The different ways of supported flattening described at docs/tomlSchema.md can be tested by loading the kitchensink catalog and configuring node4.

Example worklow
workflow (16).zip

@netlify
Copy link

netlify bot commented Mar 1, 2022

✔️ Deploy Preview for wonderful-noether-53a9e8 ready!

🔨 Explore the source changes: effed48

🔍 Inspect the deploy log: https://app.netlify.com/sites/wonderful-noether-53a9e8/deploys/6228c277d9a0150008d33c2c

😎 Browse the preview: https://deploy-preview-48--wonderful-noether-53a9e8.netlify.app

@sverhoeven sverhoeven changed the title Write nested data as flattened parameter name in toml Read/write nested data as flattened parameter name to and from toml file Mar 9, 2022
@sverhoeven sverhoeven marked this pull request as ready for review March 9, 2022 16:19
@sverhoeven sverhoeven requested a review from Peter9192 March 9, 2022 16:19
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Haven't looked at the code yet, but from playing with the preview I noticed a couple of interesting things.

Point 1

With nested indexing the layout is a bit confusing. I click the first blue +, which effectively adds an empty nested array. As a user, the only visual feedback I get is the appearance of another blue + and a minus.

image

Since adding an empty array doesn't make much sense, does it make sense to automatically also create the first item in the nested array (and perhaps fill it with defaults or so, see #54)?

Point 2

The behavior when leaving boxes empty is also a bit tricky:

image

Note that 1_1 is skipped in the toml file, since it is apparently a default value, but the fact that the count then starts at 1_2 suggests that 1_1 is still significant. But what happens with the last one? It's also pruned from the toml file, but it can never be known that there would have been a 2_2 as well.

Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Looks very good, apart from my previous concern about the trailing empty array items I think it is good to go.

Comment on lines +56 to +58
param_1 = 11
param_2 = 22
param_3 = 33
Copy link
Contributor

Choose a reason for hiding this comment

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

11, 22, and 33 are just example numbers, but due to their structure and correspondence to the params, it took me some time to realize they might as well be random other numbers. Perhaps we could consider making them a bit more arbitrary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I chose those number to easily map the indices to the values. Using arbitrary numbers would make it harder to investigate any errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this is the documentation, not testing code or so

docs/tomlSchema.md Show resolved Hide resolved
src/toml.ts Show resolved Hide resolved
src/toml.test.ts Show resolved Hide resolved
@sverhoeven
Copy link
Member Author

Thanks for the review.

For point 1 the table widget in #53 and 2d matrix widget #56 will help.

For point 2 trailing items which are equal to defaults and absent in toml is something the app running the toml needs to deal with.

@sverhoeven sverhoeven merged commit 59cec9e into main Mar 22, 2022
@sverhoeven sverhoeven deleted the write-expandable2toml branch March 22, 2022 11:37
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