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

Fix(#1027): Improve client models by reordering fields + forbidding extra args #1032

Merged
merged 2 commits into from Jan 26, 2022

Conversation

dcfidalgo
Copy link
Contributor

Closes #1027

With this PR the client models raise an error when extra arguments are provided. The error message is not the clearest though, but i did not find a way to modify this. @frascuchon Do you know how to customize validation error messages in pydantic?

It also fixes the order of the fields in the models. This is important when converting the records into a DataFrame/Dataset, so that when displaying a DataFrame the order of the columns is more or less meaningful.

@dcfidalgo dcfidalgo changed the title Feat/improve client models Fix(#1024): Improve client models by reordering fields + forbidding extra args Jan 25, 2022
@dcfidalgo dcfidalgo changed the title Fix(#1024): Improve client models by reordering fields + forbidding extra args Fix(#1027): Improve client models by reordering fields + forbidding extra args Jan 25, 2022
@dcfidalgo dcfidalgo added the type: bug Indicates an unexpected problem or unintended behavior label Jan 25, 2022
@dcfidalgo dcfidalgo self-assigned this Jan 25, 2022
@dcfidalgo dcfidalgo added this to In progress in Release via automation Jan 25, 2022
@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #1032 (a3dd4e3) into master (7667ae8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1032      +/-   ##
==========================================
+ Coverage   95.19%   95.21%   +0.01%     
==========================================
  Files         115      115              
  Lines        4330     4343      +13     
==========================================
+ Hits         4122     4135      +13     
  Misses        208      208              
Flag Coverage Δ
pytest 95.21% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/rubrix/client/models.py 98.80% <100.00%> (+0.21%) ⬆️

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 7667ae8...a3dd4e3. Read the comment docs.

Copy link
Member

@frascuchon frascuchon left a comment

Choose a reason for hiding this comment

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

You should raise your own exceptions. See here

I think we should make some effort to parse errors from pydantic validation, and make them human readable

Release automation moved this from In progress to Review Jan 25, 2022
@dcfidalgo dcfidalgo merged commit c1e32d1 into master Jan 26, 2022
Release automation moved this from Review to Done Jan 26, 2022
@dcfidalgo dcfidalgo deleted the feat/improve_client_models branch January 26, 2022 09:40
@frascuchon frascuchon moved this from Done to Release Ready in Release Jan 31, 2022
frascuchon pushed a commit that referenced this pull request Jan 31, 2022
…xtra args (#1032)

* feat: introduce RootValidators + correct order

* feat: forbid extras + test

(cherry picked from commit c1e32d1)
@frascuchon frascuchon removed this from Release Ready in Release Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Records] TextClassificationRecord does not raise error passing
2 participants