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

Replace basemodel with dataclass in event.py #5754

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

jonathan-eq
Copy link
Contributor

@jonathan-eq jonathan-eq commented Jul 31, 2023

Issue
Resolves #2813

Approach
Replaced event.py classes based on Pydantic.BaseModel with @dateclass

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Updated documentation
  • Ensured new behaviour is tested

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@jonathan-eq jonathan-eq self-assigned this Jul 31, 2023
@xjules
Copy link
Contributor

xjules commented Jul 31, 2023

I'm wondering whether this one should include other occurrences of BaseModel as well?

(py38-ert) [jparu@be-lx101262 ert]$ grep -irn "from pydantic import BaseModel"
src/ert/ensemble_evaluator/event.py:3:from pydantic import BaseModel
src/ert/ensemble_evaluator/snapshot.py:12:from pydantic import BaseModel
src/ert/_c_wrappers/analysis/configuration.py:5:from pydantic import BaseModel, conlist, root_validator, validator
src/ert/parsing/config_schema_item.py:5:from pydantic import BaseModel
src/ert/storage/local_storage.py:22:from pydantic import BaseModel
src/ert/storage/local_ensemble.py:13:from pydantic import BaseModel

@jonathan-eq jonathan-eq force-pushed the replace-basemodel-with-dataclass-in-eventpy branch from c6492de to 326e5e3 Compare July 31, 2023 13:31
Replaced event classes based on Pydantic.BaseModel with @dateclass
@jonathan-eq jonathan-eq force-pushed the replace-basemodel-with-dataclass-in-eventpy branch from b8c7188 to c7106aa Compare August 1, 2023 06:57
@hnformentin hnformentin requested a review from xjules August 1, 2023 07:09
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #5754 (c7106aa) into main (1f4d93f) will decrease coverage by 0.87%.
Report is 339 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5754      +/-   ##
==========================================
- Coverage   80.49%   79.63%   -0.87%     
==========================================
  Files         360      362       +2     
  Lines       22202    23177     +975     
  Branches      824     1399     +575     
==========================================
+ Hits        17871    18456     +585     
- Misses       4083     4347     +264     
- Partials      248      374     +126     
Files Changed Coverage Δ
src/ert/ensemble_evaluator/event.py 100.00% <100.00%> (ø)

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@berland
Copy link
Contributor

berland commented Sep 7, 2023

  • Upper-case first letter of title, it will probably go into the release notes.
  • 'Added appropriate release note labels' is ticked, but it is not done..

@jonathan-eq jonathan-eq changed the title replace basemodel with dataclass in event.py Replace basemodel with dataclass in event.py Sep 19, 2023
@jonathan-eq jonathan-eq added good first issue improvement Something nice to have, that will make life easier for developers or users or both. release-notes:skip If there should be no mention of this in release notes labels Sep 19, 2023
@jonathan-eq
Copy link
Contributor Author

I'm wondering whether this one should include other occurrences of BaseModel as well?

(py38-ert) [jparu@be-lx101262 ert]$ grep -irn "from pydantic import BaseModel"
src/ert/ensemble_evaluator/event.py:3:from pydantic import BaseModel
src/ert/ensemble_evaluator/snapshot.py:12:from pydantic import BaseModel
src/ert/_c_wrappers/analysis/configuration.py:5:from pydantic import BaseModel, conlist, root_validator, validator
src/ert/parsing/config_schema_item.py:5:from pydantic import BaseModel
src/ert/storage/local_storage.py:22:from pydantic import BaseModel
src/ert/storage/local_ensemble.py:13:from pydantic import BaseModel

The other instances make use of class methods included with BaseModel

Copy link
Contributor

@berland berland left a comment

Choose a reason for hiding this comment

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

LGTM

@berland berland added release-notes:maintenance Automatically categorise as maintenance change in release notes and removed release-notes:skip If there should be no mention of this in release notes labels Sep 21, 2023
@jonathan-eq jonathan-eq enabled auto-merge (squash) September 21, 2023 06:06
auto-merge was automatically disabled September 21, 2023 07:51

Pull request was closed

@jonathan-eq jonathan-eq reopened this Sep 21, 2023
@jonathan-eq jonathan-eq enabled auto-merge (squash) September 21, 2023 07:55
@jonathan-eq jonathan-eq merged commit 7b6b10b into main Sep 21, 2023
72 of 73 checks passed
@jonathan-eq jonathan-eq deleted the replace-basemodel-with-dataclass-in-eventpy branch September 21, 2023 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue improvement Something nice to have, that will make life easier for developers or users or both. release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace classes based on BaseModel with @dataclass where appropriate
4 participants