8000 Support complex number by changhc · Pull Request #9654 · pydantic/pydantic · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@changhc
Copy link
Contributor
@changhc changhc commented Jun 13, 2024

Change Summary

Support complex numbers.

Related issue number

Closes #8555

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Also, upgrade to pydantic-core v2.22.0, which adds support for complex schemas. We've bundled in some other patches along with this release, such as removing xfails associated with tests that this release fixed!

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Jun 13, 2024
@sydney-runkle sydney-runkle added relnotes-feature and removed relnotes-fix Used for bugfixes. labels Jun 13, 2024
@sydney-runkle
Copy link
Contributor

Hi @changhc,

Great start here - looking good.

Maybe we could add a section to the docs as well with some usage patterns.

@sydney-runkle
Copy link
Contributor

I left some notes on next steps in the pydantic-core PR :).

@sydney-runkle sydney-runkle added the awaiting author revision awaiting changes from the PR author label Jun 20, 2024
@codspeed-hq
Copy link
codspeed-hq bot commented Jun 22, 2024

CodSpeed Performance Report

Merging #9654 will not alter performance

Comparing changhc:implement-complex (bb4441a) with main (f5d6acf)

Summary

✅ 17 untouched benchmarks

@sydney-runkle
Copy link
Contributor

@changhc, could you provide an example in pydantic where the error type is complex_str_parsing?

@sydney-runkle sydney-runkle removed the awaiting author revision awaiting changes from the PR author label Aug 15, 2024
Copy link
Contributor
@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Looks good, pending revision of the complex_str_parsing error docs in another PR!

@sydney-runkle sydney-runkle enabled auto-merge (squash) August 15, 2024 17:28
@sydney-runkle
Copy link
Contributor

Probably could use some more docs here as well. I can handle that and will create an associated issue.

@sydney-runkle sydney-runkle disabled auto-merge August 15, 2024 19:11
@changhc
Copy link
Contributor Author
changhc commented Aug 15, 2024

Hi @sydney-runkle, I've updated the error doc. The test case that failed with pypy is now skipped. A fix is merged into pypy master branch but not yet released. I'm trying to fix the failed mypy test.

@@ -1,56 +1,56 @@
from pydantic import BaseModel, Field
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what is actually changed in this file. Everything looks identical. It was "updated" after I ran make test-mypy-update-all.

@github-actions
Copy link
Contributor
github-actions bot commented Aug 15, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  pydantic
  json_schema.py
  pydantic/_internal
  _generate_schema.py
  _known_annotated_metadata.py
Project Total  

This report was generated by python-coverage-comment-action

@sydney-runkle
Copy link
Contributor

@changhc,

Awesome find! Great work on this feature. We'll be sure to celebrate this in our upcoming release!!

I recall you mentioned you might be interested in working on some other issues. If this is still the case, I can point you towards some fun ones!

@sydney-runkle sydney-runkle enabled auto-merge (squash) August 15, 2024 22:34
@changhc
Copy link
Contributor Author
changhc commented Aug 15, 2024

Thank you for the review! Yes I would love to look into other issues.

By the way, how much does the team care about test coverage? After reading the test coverage report, I'm now adding some tests to cover the missing lines, though I'm not sure if they actually help since the tests to be added are like simply checking if the json schema of a model matches the expected schema.

@sydney-runkle sydney-runkle disabled auto-merge August 15, 2024 22:42
@sydney-runkle
Copy link
Contributor

By the way, how much does the team care about test coverage? After reading the test coverage report, I'm now adding some tests to cover the missing lines, though I'm not sure if they actually help since the tests to be added are like simply checking if the json schema of a model matches the expected schema.

We care a decent amount about coverage - more tests sound great. Happy to merge after we add those. We generally try to keep coverage above 90%, but closer to 100% would be great.

Thank you for the review! Yes I would love to look into other issues.

Awesome, I'll ping you with some fun ones soon.

@sydney-runkle
Copy link
Contributor

@changhc, as promised, some fun issues:

@changhc
Copy link
Contributor Author
changhc commented Aug 15, 2024

The missing line in the previous report is now covered.

@sydney-runkle sydney-runkle merged commit 58abc3d into pydantic:main Aug 16, 2024
@sydney-runkle
Copy link
Contributor

@changhc, another fun one: #9731

@changhc changhc deleted the implement-complex branch August 16, 2024 21:36
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.

Standard complex number not supported!?

2 participants

0