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

Line with SerializedHierarchy #793

Closed
wants to merge 2 commits into from

Conversation

tuxbotix
Copy link
Contributor

@tuxbotix tuxbotix commented Apr 6, 2024

Introduced Changes

It relates to #147, but Fixing Line first.

I'm not convinced if the naming is the best, but 0 or 1 are invalid field names making them impossible to use.

Fixes #

ToDo / Known Issues

..

Ideas for Next Iterations (Not This PR)

Complete coverage for : #147
Or find an alternative approach to handle Tuple structs with SerializedHierarchy!

How to Test

Describe how to test your changes. (For the reviewer)

@tuxbotix tuxbotix changed the title Line with SerializedHierarchy (Tuple to struct change) Line with SerializedHierarchy Apr 6, 2024
@tuxbotix tuxbotix enabled auto-merge April 6, 2024 17:35
Copy link
Contributor

@knoellle knoellle left a comment

Choose a reason for hiding this comment

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

Why don't you just manually implement the trait for Line?
Then you wouldn't have to any of the usages.

@tuxbotix
Copy link
Contributor Author

Why don't you just manually implement the trait for Line? Then you wouldn't have to any of the usages.

@knoellle I forgot to mention, #797 can kill this MR as that introduces SerializeHierarchy for enum structs. Could you please take a look and lets figure out what's the best option?

A workaround option is to add leaf annotation to the line types but I personally prefer the above MR as the best option.

@schmidma
Copy link
Member

schmidma commented May 5, 2024

Superseded by #987

@schmidma schmidma closed this May 5, 2024
auto-merge was automatically disabled May 5, 2024 18:37

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants