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

Add block ordering and block comment saving #7

Closed
wants to merge 2 commits into from

Conversation

JoelLucaAdams
Copy link
Collaborator

Researchers have requested the ability to store multiple blocks with similar names (e.g. multiple control blocks as seen in the updated cone.deck). There has also been significant interest in storing newline and inline comments from a block. The user experience remains mostly unchanged unless blocks contain comments. The only visible difference is that the loads function now returns a tuple instead of just the dictionary.

Handling of Blocks with the Same Name

Blocks that share the same name but do not have a name parameter are now stored as separate entries in an array, rather than being merged into the same dictionary.

loads and load Function Updates

  • The loads (and consequently load) functions now return a tuple containing both the deck in dictionary format and a block_order list.
  • The block_order maintains the order in which blocks are loaded. It uses the following notation:
    • {block_name}: For blocks with a unique name.
    • {block_name}:{sub_block}: For blocks that have the same name and contain a name field (sub-blocks).
    • {block_name}_{i}: For blocks with the same name but no name field, where i is a zero-based index.

Comment Handling

Comments within blocks are now stored using the following notation:

  • Standalone comments (on new lines) are indexed as comment_{i} where i starts at 0.
  • Inline comments (after a key-value pair) are indexed as {key}_inline_comment_{i} where key is the key and i starts at 0.

Testing:

Tests have been updated to reflect the new behaviour.

@LiamPattinson
Copy link

I'm not so sure about returning both a dict and a list of ordering info. It's a pretty big breaking change, and users might find it annoying to keep track of both of these independently. Could we instead implement a new class that implements collections.abc.MutableMapping? The block ordering could then be implemented as an internal feature of the class, while the class itself would behave just like a dict. It would still break user code that does things like isinstance(my_deck, dict), but as long as the users are duck-typing properly they shouldn't notice any difference.


It also doesn't look like the comment-tracking and same-name-blocks issues are related (correct me if I'm wrong!). Would it be possible to spin one of these off into a new PR? As this is only one commit, it might take some effort with interactive rebasing and git add --patch to split the features apart. Tools like lazygit can help with this, and I'm sure @ZedThree also has some good suggestions.

@ZedThree
Copy link
Member

I'm not quite sure I understand the purpose of the multiple blocks here -- does EPOCH handle them separately somehow?

@JoelLucaAdams
Copy link
Collaborator Author

I'm going to be completely honest here folks, I'm not sure where or how I got into my head that we can have multiple blocks that have the same name but no name parameter. I'm going to take this one back to the drawing board and remove multiblock and transfer to using collections.abc.MutableMapping.

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