Conversation
|
Would you mind pulling down and trying out Thanks!!! |
|
Sure, I’ll try to test before Monday and will follow up with feedback.
…On Fri, Dec 20, 2024 at 5:01 PM Chris Newhouse ***@***.***> wrote:
@cyrusradfar <https://github.com/cyrusradfar> and @rgimen3z
<https://github.com/rgimen3z>
Would you mind pulling down and trying out 5.0.0b1 to make sure that
things are working as expected for you? I'd feel much better if you two
were able to make sure that this was working well for you before I released
it.
Thanks!!!
—
Reply to this email directly, view it on GitHub
<#68 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABASCWD3VVVN4DUBKKBD2NL2GS4WTAVCNFSM6AAAAABT7YKKVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJXHEZDQMZRHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
This message is intended only for the individual or entity to which it is
addressed. It may contain privileged, confidential information which is
exempt from disclosure under applicable laws. If you are not the intended
recipient, you are strictly prohibited from disseminating or distributing
this information (other than to the intended recipient) or copying this
information. If you have received this communication in error, please
notify us immediately by e-mail or by telephone at the above number. Thank
you.
|
pyproject.toml
Outdated
| # urllib must be pinned to <2 due to an issue with the `requests_toolbelt` | ||
| # dependency in `gql`. | ||
| # Similar: https://github.com/apache/arrow/commit/4295e3dd06f9f507f75ccc780eaca16a6b621a02 | ||
| urllib3 = "<2" |
There was a problem hiding this comment.
could we unpin this? we have a couple dependencies that require urllib3>=2 that we'd have to downgrade. I believe I was able to remove the dependency in #65 and things still worked fine, but I don't have all the context
| pydocstyle = "^6.3.0" | ||
| pylint = "^2.16.2" | ||
|
|
||
| # FIXME: Upgrading mypy will require updates to aliased fields. e.g. |
There was a problem hiding this comment.
i noticed this too, I think these aliases are just for serialization, and updating to serialization_alias makes mypy happy (also see: https://github.com/anvilco/python-anvil/pull/65/files)
|
@newhouse after reviewing and testing, it looked good. I saw a opportunity to clean up what I added and re-tested. Placed that here. Otherwise, I'm good to go and would agree that the less we can pin on specific versions the better but that's non-blocking feedback. Thanks for following up and taking this to the finish line. |
Remove unnecessary file length calculation in FileCompatibleBaseModel and Debug Code
cyrusradfar
left a comment
There was a problem hiding this comment.
👍 Approval
This is a solid update that modernizes the codebase and improves developer experience.
Key improvements I'm seeing:
- Upgrades Python version support (dropping 3.7, adding 3.11)
- Updates to Pydantic v2 syntax and best practices (yay!)
- Adds helpful example code and documentation
- Improves file handling with new
FileCompatibleBaseModel - Adds comprehensive tests for file handling
💡 Non-blocking Suggestions
-
Documentation
- Consider adding a CHANGELOG.md entry for this significant version bump, last is from July
- The new CLI example is great - maybe add it to the README.md highlights as well
-
Code Improvements
- The commented out
json_deserializecode in http.py could be removed - Consider adding type hints for the new file handling methods in models.py
- The commented out
-
Testing
- The new file handling tests are thorough, but we could add a few edge cases:
- Empty files
- Very large files
- Files with special characters in names
- The new file handling tests are thorough, but we could add a few edge cases:
Overall
Lovely work modernizing the codebase while maintaining backward compatibility. The improved file handling and examples will make integration much easier for developers.
There's always more to do -- ready to merge! 🚀
|
I have removed the pegging of |
1 similar comment
|
I have removed the pegging of |
|
I have removed the pegging of |
pulled and tested locally with no new issues. Thanks for adding the changelog |
|
@rgimen3z Im gonna merge this and it has been published as |
5.0.0 that updates Pydantic and other dependencies, while addressing some breaking changes.