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

Add conset #1627

Merged
merged 7 commits into from Jul 3, 2020
Merged

Add conset #1627

merged 7 commits into from Jul 3, 2020

Conversation

patrickkwang
Copy link

@patrickkwang patrickkwang commented Jun 11, 2020

Change Summary

This adds conset(). The functional additions are exactly analogous to the machinery for conlist(). The tests are also exactly analogous to those for conlist(), with the addition of a single assertion ensuring that length checks are occurring after casting the input to a set, i.e. set([1, 1, 1, 2, 2]) satisfieds max_items=4.

Related issue number

#1623

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)

@patrickkwang
Copy link
Author

I'm puzzled. The checks that I fixed in 7dcc248 are still failing, as if nothing changed. Any ideas?

@samuelcolvin
Copy link
Member

It looks like linting is failing because lines are too long.

Nothing stranger I can see.

@patrickkwang
Copy link
Author

If I'm reading things correctly, the latest checks indicate that types.py:8 has 121 characters:
https://github.com/samuelcolvin/pydantic/pull/1627/checks?check_run_id=763653036#step:5:9

But the current diff shows that types.py:8 has 20 characters:
https://github.com/samuelcolvin/pydantic/pull/1627/files#diff-470d0730db2e82eaaf27ad3655a667b3R8

I must be misinterpreting something.

@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #1627 into master will decrease coverage by 0.10%.
The diff coverage is 95.65%.

@@             Coverage Diff             @@
##            master    #1627      +/-   ##
===========================================
- Coverage   100.00%   99.89%   -0.11%     
===========================================
  Files           21       21              
  Lines         3796     3845      +49     
  Branches       752      763      +11     
===========================================
+ Hits          3796     3841      +45     
- Misses           0        2       +2     
- Partials         0        2       +2     
Impacted Files Coverage Δ
pydantic/types.py 99.57% <92.85%> (-0.43%) ⬇️
pydantic/errors.py 100.00% <100.00%> (ø)
pydantic/fields.py 100.00% <100.00%> (ø)
pydantic/schema.py 100.00% <100.00%> (ø)
pydantic/networks.py 99.21% <0.00%> (-0.79%) ⬇️
pydantic/typing.py 100.00% <0.00%> (ø)
pydantic/validators.py 100.00% <0.00%> (ø)
... and 3 more

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 5e82689...8f608e1. Read the comment docs.

@samuelcolvin
Copy link
Member

not sure what was wrong with linting, it's now failing on something else.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise I think looks good.

changes/1623-patrickkwang.md Outdated Show resolved Hide resolved
tests/test_schema.py Outdated Show resolved Hide resolved
tests/test_schema.py Outdated Show resolved Hide resolved
Patrick Wang and others added 3 commits June 27, 2020 10:57
@samuelcolvin
Copy link
Member

awesome, thank you.

@samuelcolvin samuelcolvin merged commit dca9855 into pydantic:master Jul 3, 2020

def test_constrained_set_default():
class ConSetModelMax(BaseModel):
v: conset(int) = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This test falils on my side indicating that is not set, but rather a dict.

Copy link
Member

Choose a reason for hiding this comment

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

Matter is failing, maybe that's why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Matter?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, master.

Copy link
Member

Choose a reason for hiding this comment

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

This is very weird, as per #1682 this can be fixed by setting the default to a set.

However this should still pass since we don't have validate_always=True - the default value (an empty dict) should be okay.

The question is: what has changed on master since this was started that's causing this to fail when merged, and could that be a smoke signal to a bigger problem somewhere?

Copy link
Contributor

@Bobronium Bobronium Jul 4, 2020

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes I just saw that. I was investigating, then had to stop. I'll try to look later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants