Skip to content

Conversation

@agarny
Copy link
Contributor

@agarny agarny commented Nov 20, 2025

Fixes issue #1339.

@agarny agarny force-pushed the issue1339 branch 6 times, most recently from 4c07dec to ed39816 Compare November 24, 2025 00:37
@agarny agarny force-pushed the issue1339 branch 2 times, most recently from 877deee to 9647cd1 Compare November 24, 2025 06:51
@hsorby hsorby changed the title Add a GeneratorContext class to manage un/tracked variables Add a GeneratorVariableTracker class to manage un/tracked variables Nov 25, 2025
@agarny
Copy link
Contributor Author

agarny commented Nov 25, 2025

Note that I am not done yet...

@nickerso
Copy link
Contributor

Why the change from GeneratorContext to GeneratorVariableTracker? Some useful commit messages would be great to start seeing. I am not a fan of that change and really struggling to see a need for this at all from the description of #1339...starting to sound more and more like some internal data management that needs no visible presence in the API.

@agarny
Copy link
Contributor Author

agarny commented Nov 25, 2025

Why the change from GeneratorContext to GeneratorVariableTracker? Some useful commit messages would be great to start seeing. I am not a fan of that change and really struggling to see a need for this at all from the description of #1339...starting to sound more and more like some internal data management that needs no visible presence in the API.

GeneratorContext is a bit too generic while GeneratorVariableTracker describes what the class is really about.

In the end, the idea is for the Generator class not to hold anything (i.e. a profile and a variable tracker are to be passed to interfaceCode() and implementationCode() rather than applied to a Generator object using setProfile() and setVariableTracker()).

Now, I agree that this is getting a bit out of hands and this is why I am finishing this cosmetic work (!!) now and this is it.

So that we don't have to pass it as a parameter the whole time.
Copy link
Contributor

@hsorby hsorby left a comment

Choose a reason for hiding this comment

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

I like this interface for the generator, I think it looks great.

Copy link
Contributor

@nickerso nickerso left a comment

Choose a reason for hiding this comment

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

I don't think this is a good change, but happy to let it through. I assume documentation and tutorial updates are imminent that will help users transition our code to all these new APIs.

@nickerso nickerso merged commit c51381a into cellml:main Nov 25, 2025
14 checks passed
@agarny agarny deleted the issue1339 branch November 25, 2025 21:30
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.

3 participants