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

Rename fsapi.FlowsheetExport dict() method and model_objects field to resolve Pydantic 2.0 warnings #1346

Merged
merged 5 commits into from
May 9, 2024

Conversation

lbianchi-lbl
Copy link
Contributor

@lbianchi-lbl lbianchi-lbl commented Mar 28, 2024

Fixes/Resolves: #1344

(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)

Summary/Motivation:

Changes proposed in this PR:

  • Replace calls to .dict() with .model_dump()
    • This is a straightforward change since it's just tracking changes in the Pydantic API
  • Rename model_objects field to exports due to the model_* namespace being reserved by Pydantic
    • This could also be resolved by "unreserving" the model_* namespace
    • However, I think we should not do that if possible, otherwise it could be confusing for fields/methods where model_ refers to completely different things coexisting in the same class
    • This is a more meaningful change, and the choice of alternative name should be discussed
    • I chose exports on the basis that the item class is named ModelExport
    • Everyone should feel free to come up with feedback and/or alternatives, but I think the choice should ultimately be up to @dangunter and @MichaelPesce
  • Before merging this PR, we should thoroughly check whether the same changes should be applied to watertap-org/watertap-ui as well

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@lbianchi-lbl lbianchi-lbl marked this pull request as ready for review April 26, 2024 12:40
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.84%. Comparing base (38735ec) to head (80d2748).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1346   +/-   ##
=======================================
  Coverage   93.84%   93.84%           
=======================================
  Files         339      339           
  Lines       35890    35890           
=======================================
  Hits        33682    33682           
  Misses       2208     2208           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@MichaelPesce MichaelPesce left a comment

Choose a reason for hiding this comment

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

@lbianchi-lbl the changes look fine to me. We'll have to update the UI accordingly - essentially just replace all references of fs_exp.model_objects with fs_export.exports. I've created a PR for that here

@dangunter
Copy link
Collaborator

I like the name "exports"

Copy link
Collaborator

@dangunter dangunter left a comment

Choose a reason for hiding this comment

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

This is good, thank you Ludovico for your help dealing with Pydantic.

@lbianchi-lbl
Copy link
Contributor Author

This is good, thank you Ludovico for your help dealing with Pydantic.

I think @MichaelPesce might have gotten the short end of the stick based on the sizes of the changes in watertap-org/watertap-ui#114 😅

@lbianchi-lbl lbianchi-lbl merged commit e85b3f6 into watertap-org:main May 9, 2024
26 checks passed
lbianchi-lbl pushed a commit to watertap-org/watertap-ui that referenced this pull request May 10, 2024
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.

Resolve Pydantic deprecation warnings
3 participants