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.

Sign up for GitHub

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

Jump to bottom

[Refactor] struct CustomSerialized contains Yaml (+ CustomType) #331

Merged
merged 6 commits into from
Aug 2, 2023
Merged

[Refactor] struct CustomSerialized contains Yaml (+ CustomType) #331

merged 6 commits into from
Aug 2, 2023

Conversation

acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Aug 2, 2023

Tests in progress but see if this agrees with what you were thinking.

  • Remove ContainerValue::Opaque (which previously contained YAML)
  • Do not move ConstValue::Opaque into ContainerValue (rather, remove the TODO saying that). ContainerValue will be shared with TypeArg, so keep dyn CustomConst well away from that.

@acl-cqc acl-cqc requested a review from ss2165 August 2, 2023 11:03
@acl-cqc acl-cqc force-pushed the refactor/yaml_const branch 3 times, most recently from b2689ed to e16cdb4 Compare August 2, 2023 12:17
ss2165
ss2165 approved these changes Aug 2, 2023
View reviewed changes
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

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

Yep this is what I was picturing. Though YamlValue may want a more descriptive name. Maybe SerialValue?

@acl-cqc acl-cqc changed the title [Refactor] YamlConst struct implements CustomConst [Refactor] struct CustomSerialized contains Yaml (+ CustomType) Aug 2, 2023
@acl-cqc acl-cqc enabled auto-merge August 2, 2023 14:22
@acl-cqc acl-cqc added this pull request to the merge queue Aug 2, 2023
Merged via the queue into main with commit 5d2f09e Aug 2, 2023
5 checks passed
@acl-cqc acl-cqc deleted the refactor/yaml_const branch August 2, 2023 14:25
@acl-cqc acl-cqc mentioned this pull request Aug 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ss2165 ss2165 ss2165 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@acl-cqc @ss2165