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

refactor: update the variable interfaces. #8388

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

gonfunko
Copy link
Contributor

The basics

The details

Proposed Changes

This PR revamps the new variable interfaces in response to issues encountered during implementation. Specifically:

  • save and load are moved from IVariableMap to IVariableModel. In the case where somebody wants to add additional properties to their variables, this means they need only subclass/implement IVariableModel, and not also have to replace the variable map solely to handle serialization.
  • getWorkspace() is added to IVariableModel; the existing VariableModel had this property, and it's often necessary to determine which workspace a variable is associated with.
  • addVariable() is added to IVariableMap. Again, this allows for simple variables-with-extra-fields without having to subclass the VariableMap in order to change the method signature of createVariable to accomodate new properties; the custom variable can simply be instantiated, and then added to the vanilla VariableMap.

In order to get things to build, this PR updates VariableModel to conform to the new interface and register itself, and updates variable serialization to use the new save and load methods it introduces rather than hardcoding serialization.

@gonfunko gonfunko merged commit 32f8e24 into google:rc/v12.0.0 Jul 18, 2024
11 checks passed
@gonfunko gonfunko deleted the variable-interfaces-revamp branch July 18, 2024 18:01
@gonfunko
Copy link
Contributor Author

Fixed #8077

@gonfunko
Copy link
Contributor Author

Also fixed #8078.

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

Successfully merging this pull request may close these issues.

2 participants