Skip to content
This repository has been archived by the owner on Dec 16, 2024. It is now read-only.

attr -> pydantic #154

Merged
merged 19 commits into from
Nov 27, 2024
Merged

attr -> pydantic #154

merged 19 commits into from
Nov 27, 2024

Conversation

Stoops-ML
Copy link
Collaborator

@Stoops-ML Stoops-ML commented Nov 25, 2024

Replace attrs with pydantic.

  • Type checking of every argument of every class
  • Simplifies dumping to json string
  • Bumps version to 2.0.0 as there are breaking changes
  • Supports Python 3.11 upwards due to using type hints Self and StrEnum. StrEnum is very useful as all classes in enums.py can be used with auto() to return the exact member. Self isn't critical and less clean alternatives can be found for replacing StrEnum in order to support Pythons <3.11.
  • Tests have been modified minimally to account for small changes in output (e.g. spaces in how pydantic performs the model dump). All tests pass in this current PR for Pythons 3.11+.

@astrojuanlu Let me know your thoughts.

@Stoops-ML Stoops-ML marked this pull request as draft November 25, 2024 19:58
@Stoops-ML Stoops-ML marked this pull request as ready for review November 25, 2024 20:01
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 99.30556% with 5 lines in your changes missing coverage. Please review.

Project coverage is 99.44%. Comparing base (56632e3) to head (1b4ead7).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/czml3/types.py 98.16% 4 Missing ⚠️
src/czml3/enums.py 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #154      +/-   ##
==========================================
+ Coverage   99.28%   99.44%   +0.16%     
==========================================
  Files          12       11       -1     
  Lines         837      898      +61     
==========================================
+ Hits          831      893      +62     
+ Misses          6        5       -1     

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

@astrojuanlu
Copy link
Member

Oh I love this idea! I think it makes a ton of sense. Cannot look into the code just yet but ping me again when the CI is green

@Stoops-ML
Copy link
Collaborator Author

What's your decision on typing? If we're using Self and StrEnum then we're limited to Python 3.11+ (see my original post).

@Stoops-ML Stoops-ML enabled auto-merge (squash) November 25, 2024 21:33
@astrojuanlu
Copy link
Member

If we're using Self and StrEnum then we're limited to Python 3.11+

Aren't there backports in typing_extensions or similar?

@Stoops-ML
Copy link
Collaborator Author

Python 3.7 support has been dropped as Pydantic>=2.10.0 doesn't support it.

@astrojuanlu

@astrojuanlu
Copy link
Member

Good for me, 3.7 is EOL anyway. So is 3.8, by the way. Let's drop both.

I see you replaced | with Union everywhere, is that necessary?

@Stoops-ML
Copy link
Collaborator Author

Python 3.8 dropped.

pydantic doesn't work with | on Python 3.9 even with from __future__ import annotations specified. Someone noted the problem on SO here.

Want to drop Python 3.9 or implement Union?

@Stoops-ML Stoops-ML disabled auto-merge November 27, 2024 13:35
@Stoops-ML Stoops-ML enabled auto-merge (squash) November 27, 2024 13:36
@astrojuanlu
Copy link
Member

Hm interesting. Well, let's drop Python 3.9 then!

@Stoops-ML
Copy link
Collaborator Author

All checks passed!

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

LGTM!

@Stoops-ML Stoops-ML merged commit 78fcbe9 into main Nov 27, 2024
7 checks passed
@Stoops-ML Stoops-ML deleted the pydantic branch November 27, 2024 21:44
@Stoops-ML Stoops-ML mentioned this pull request Nov 28, 2024
9 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants