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

✅ More RootModel tests #5943

Merged
merged 1 commit into from
May 31, 2023
Merged

✅ More RootModel tests #5943

merged 1 commit into from
May 31, 2023

Conversation

lig
Copy link
Contributor

@lig lig commented May 30, 2023

Change Summary

Add tests illustrating RootModel issues

Related issue number

None

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @samuelcolvin

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 30, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: c8be7a6
Status: ✅  Deploy successful!
Preview URL: https://e7cab1e3.pydantic-docs2.pages.dev
Branch Preview URL: https://more-root-model.pydantic-docs2.pages.dev

View logs

@lig lig force-pushed the more-root-model branch 5 times, most recently from 8f10588 to a8186f0 Compare May 31, 2023 09:42
@lig lig marked this pull request as ready for review May 31, 2023 09:50
@lig
Copy link
Contributor Author

lig commented May 31, 2023

please review

Comment on lines +254 to +256
# TODO: Should this be an `AttributeError`?
with pytest.raises(ValueError, match='other_attr'):
m.other_attr = 10
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be ValueError, but not sure if it should be something different than AttributeError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In vanilla Python it's AttributeError.

>>> class A:
...     __slots__ = ()
...
>>> a = A()
>>> a.foo = 42
Traceback (most recent call last):
  Cell In[10], line 1
    a.foo = 42
AttributeError: 'A' object has no attribute 'foo'

Copy link
Member

Choose a reason for hiding this comment

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

Are you going to use AttributeError on this PR then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in this one. Maybe later. That is why I did put TODO here:)

tests/test_root_model.py Show resolved Hide resolved
tests/test_root_model.py Show resolved Hide resolved
@Kludex
Copy link
Member

Kludex commented May 31, 2023

please update

@lig
Copy link
Contributor Author

lig commented May 31, 2023

@Kludex please review

@lig lig merged commit 33fae06 into main May 31, 2023
53 checks passed
@lig lig deleted the more-root-model branch May 31, 2023 14:19
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.

None yet

3 participants