Skip to content

Conversation

@ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Aug 16, 2025

Purpose

Created from #450.

Realized that we effectively broke sol_inform in #437 because we were previously using a dict. This is now fixed by implementing __getitem__, and I think we should just make a patch release ASAP, this oversight is probably going to break pretty much everyone's code...

Expected time until merged

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Checklist

  • I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@ewu63 ewu63 requested a review from a team as a code owner August 16, 2025 17:47
@ewu63 ewu63 requested review from ArshSaja and marcomangano August 16, 2025 17:47
@ewu63 ewu63 mentioned this pull request Aug 16, 2025
13 tasks
Copy link
Collaborator

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

Good catch, the test looks good

@kanekosh kanekosh merged commit 8af45c8 into main Aug 16, 2025
25 of 27 checks passed
@kanekosh kanekosh deleted the fix-solinform-getitem branch August 16, 2025 20:51
@eirikurj
Copy link
Contributor

Little late to this, but do we want to add some deprecation warning here when accessing as a dict? In general, we could keep both features for accessing data, but I think the dataclass access is superior and the correct application here compared to the dict. Thus, we should consider removing the dict referencing in a future version. Let me know what you think.

@whophil
Copy link
Contributor

whophil commented Aug 18, 2025

Also late to the party here, but what about TypedDict?

@ewu63
Copy link
Collaborator Author

ewu63 commented Aug 18, 2025

So @eirikurj to get to your question from the other thread, I'm pretty sure anyone who wants to access the exit code (and many people presumably would) uses sol.optInform which previously was a dict and now is a dataclass, so it is a major breaking change IMO. In any case this PR patches it OK, I can add the deprecation warnings as suggested.

@whophil I'm not sure if a TypedDict will do anything better, since we already moved to a dataclass format which is probably a little cleaner, and I'm not sure if we can use TypedDict to patch the compatibility somehow.

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.

6 participants