-
-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
@ltworf I recently discovered typedload and find it a breath of fresh air compared to other libraries that don't work with e.g. standard dataclasses - thank you for making it! I see merging is blocked on getting the commit signed, I will look into how to set that up for this contribution. |
Hello, thanks for the contribution. I'm on vacation now, I'll have a look at it next week I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
overall it is good. I commented on some things that need fixing before this can be merged.
For the future, I'd prefer separate commits for typecheck, dumper, loader, and tests… or maybe the tests can be added in the same commit of the relative function.
Yes, you should add an entry in the changelog, and update to https://github.com/ltworf/typedload/blob/master/docs/supported_types.md is needed as well.
Don't worry about the signature, I can override the setting if you've never used it before.
(lambda type_: type_ in self.strconstructed, _strconstructload), | ||
(lambda type_: type_ == datetime.timedelta, _timedeltaload), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you move this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was a remnant of earlier debugging but it should not be needed, undid this, thanks for pointing it out.
1de8334
to
04a9977
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope you had a relaxing vacation! Thank you for the review.
Good point that this needs error handling, added that with tests. Updated the two docs you mentioned as well.
I addressed all the feedback in a new fixup-style commit; happy to squash it into the previous commit or squash into 3 commits (loader changes + tests, dumper changes + tests, docs changes), let me know what you prefer.
I would appreciate if you could override the setting about the signatures.
Let me know if there is anything I missed!
(lambda type_: type_ in self.strconstructed, _strconstructload), | ||
(lambda type_: type_ == datetime.timedelta, _timedeltaload), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was a remnant of earlier debugging but it should not be needed, undid this, thanks for pointing it out.
Ok, merged. Let me know if you'd like to contribute more, then I could add you to the project so you can do PR directly. Thanks :) |
Thanks! I haven't run into any other meaningful pieces to contribute yet but will let you know if I do. That being said, one minor note, I just noticed a few more places that may need to be updated to mention the new support from this PR:
I wonder if we need to have the list of supported types in 3 places and if we could consolidate some of those? In any case happy to make a PR tomorrow with whatever your preference is. |
Ah yes those should be updated as well, you want to make another pr for that? Consolidation would be good, but they appear in different spots. The readme is the "homepage" and also the text that goes on pypi, the docstring is converted on the website but also appears with |
Fair enough - made #441 to update the docs. |
No description provided.