davidhewitt / pythonize

MIT License
198 stars 28 forks source link

Expose the Pythonizer and Depythonizer #31

Closed sandhose closed 7 months ago

sandhose commented 1 year ago

This exposes the Pythonizer and Depythonizer structs, adding documentation to them and to their public methods. It also adds methods to create a Pythonizer.

Fixes #30

davidhewitt commented 1 year ago

Thanks for this, and sorry for the delayed review.

Overall I'm totally fine with adding this.

Would you be willing to add some tests for this functionality, maybe using serde_path_to_error, so that we can be sure that the pythonize public API will continue to support your use case going forward?

codecov-commenter commented 1 year ago

Codecov Report

Base: 80.99% // Head: 81.16% // Increases project coverage by +0.16% :tada:

Coverage data is based on head (6aaf26b) compared to base (d1e629d). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #31 +/- ## ========================================== + Coverage 80.99% 81.16% +0.16% ========================================== Files 4 4 Lines 1021 1030 +9 ========================================== + Hits 827 836 +9 Misses 194 194 ``` | [Impacted Files](https://codecov.io/gh/davidhewitt/pythonize/pull/31?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt) | Coverage Δ | | |---|---|---| | [src/lib.rs](https://codecov.io/gh/davidhewitt/pythonize/pull/31/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt#diff-c3JjL2xpYi5ycw==) | `100.00% <ø> (ø)` | | | [src/de.rs](https://codecov.io/gh/davidhewitt/pythonize/pull/31/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt#diff-c3JjL2RlLnJz) | `88.43% <100.00%> (ø)` | | | [src/ser.rs](https://codecov.io/gh/davidhewitt/pythonize/pull/31/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt#diff-c3JjL3Nlci5ycw==) | `86.43% <100.00%> (+0.33%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Waelwindows commented 1 year ago

What's the status on this PR? I think it's important we get this merged!

Would you be willing to add some tests for this functionality, maybe using serde_path_to_error, so that we can be sure that the pythonize public API will continue to support your use case going forward?

It's not necessary as serde_path_to_error just requires that the (de)serializer are public and implement the relevant traits from serde

Also PythonizeDefault isn't public, which breaks Pythonizer::new

davidhewitt commented 1 year ago

Still waiting for tests.

It's not necessary as serde_path_to_error just requires that the (de)serializer are public and implement the relevant traits from serde

I disagree, without tests I could easily make these types not public by accident in a future release. All a test would need to do is make an example use of serde_path_to_error. That would be sufficient to function as a guard against me removing these.

@Waelwindows, If you'd like to see this merged, perhaps you would be willing to push a copy of this PR with a test added please?

davidhewitt commented 7 months ago

Replaced by #52