Skip to content

Comments

Prep for pydantic Artifacts/Producers#71

Merged
JacobHayes merged 3 commits intogoldenfrom
pydantic-prep
Jul 27, 2021
Merged

Prep for pydantic Artifacts/Producers#71
JacobHayes merged 3 commits intogoldenfrom
pydantic-prep

Conversation

@JacobHayes
Copy link
Member

@JacobHayes JacobHayes commented Jul 27, 2021

  • Rename schema -> type to avoid pydantic collision
  • Rename {Format,Storage}.validate_artifact -> .supports
  • Make Models frozen by default and add stricter type checking (pydantic parses)

@JacobHayes JacobHayes self-assigned this Jul 27, 2021
@JacobHayes JacobHayes requested a review from mikss July 27, 2021 06:07
@codecov
Copy link

codecov bot commented Jul 27, 2021 •
edited
Loading

Codecov Report

Merging #71 (fd4a630) into golden (9cb47be) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            golden       #71   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          685       699   +14     
  Branches        79        81    +2     
=========================================
+ Hits           685       699   +14
Impacted Files Coverage Δ
src/arti/statistics/core.py 100.00% <ø> (ø)
src/arti/artifacts/core.py 100.00% <100.00%> (ø)
src/arti/formats/core.py 100.00% <100.00%> (ø)
src/arti/internal/models.py 100.00% <100.00%> (ø)
src/arti/storage/core.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cb47be...fd4a630. Read the comment docs.

mikss
mikss approved these changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@mikss mikss left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like the name change away from schema, but is there any chance of confusion that a.type and type(a) are totally different, despite seemingly similar naming? Note that this does not occur with pd.DataFrame.dtypes, since that attribute dtypes: (a) is not named type; (b) exactly corresponds to the numpy data types.

That is to say, if there's something else that is neither schema nor type, it might be worth considering (but then, of course, Type and TypeSystem and TypeAdapter would merit another look as well).

Base automatically changed from sunder to golden July 27, 2021 14:33
@JacobHayes
Copy link
Member Author

JacobHayes commented Jul 27, 2021

is there any chance of confusion that a.type and type(a) are totally different, despite seemingly similar naming?

I don't think too much(🤞), type(a) should match the stdlib a.__class__ dunder. I am a bit worried about all pydantic models having a .schema method, which in the case of Artifact (and maybe Type) is kinda ambiguous though - hopefully pydantic/pydantic#1001 goes through.

That is to say, if there's something else that is neither schema nor type, it might be worth considering (but then, of course, Type and TypeSystem and TypeAdapter would merit another look as well).

Yeah, I was trying to think of this, but nothing came up.

@JacobHayes JacobHayes changed the title Prep for pydantic for Artifacts/Producers Prep for pydantic Artifacts/Producers Jul 27, 2021
@JacobHayes JacobHayes merged commit c920e49 into golden Jul 27, 2021
@JacobHayes JacobHayes deleted the pydantic-prep branch July 27, 2021 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mikss mikss mikss approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JacobHayes JacobHayes

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JacobHayes @mikss