Skip to content

Conversation

@taion
Copy link

@taion taion commented Apr 20, 2018

This PR demonstrates injecting the JIT without adding any explicit hooks in Marshmallow. The tests in this test suite all pass, but I didn't patch the upstream vendored tests, as the configuration mechanism is now different.

This allows eliminating the custom Marshmallow fork.

@taion taion closed this Apr 20, 2018
@taion taion deleted the marshal-inject branch April 20, 2018 23:22
@taion taion restored the marshal-inject branch April 20, 2018 23:24
@taion taion reopened this Apr 20, 2018
@taion
Copy link
Author

taion commented Apr 20, 2018

Oops, accidentally deleted the branch. Didn't mean to do that. This PR is still as intended.

@taion
Copy link
Author

taion commented Apr 20, 2018

Got the benchmarks working too.

I'm not sure how to run the Marshmallow test suite with this API, though. I'm sure I could figure something out.

Still, how does this approach look? This seems easier to integrate, as it removes the "must uninstall Marshmallow" requirement.

@taion
Copy link
Author

taion commented May 13, 2018

@rowillia Any thoughts here?

@taion
Copy link
Author

taion commented Aug 16, 2018

@rowillia Ping! Does this seem like an interesting avenue to pursue?

install_requires=[
'attrs >= 17.1.0'
'attrs >= 17.1.0',
'marshmallow == 3.0.0b2',

Choose a reason for hiding this comment

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

By pinning it to == 3.0.0b2, users are forced to use this particular version of marshmallow. Can you change it to a less restrictive version bound?

Copy link
Author

Choose a reason for hiding this comment

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

i'd need to make a small patch to upstream to work with the latest v3-line releases to factor out the marshaller again

Copy link
Author

Choose a reason for hiding this comment

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

in general for pre-releases, there are no API-compatibility guarantees, so pinning a specific release is safest

Choose a reason for hiding this comment

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

Ah I see, thanks! I'm interested in using toasted-marshmallow with the upcoming marshmallow 3 as well.

Have you considered forking since this repo seems to be unmaintained?

Copy link
Author

Choose a reason for hiding this comment

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

i'd prefer not to do so. i don't understand the core code here well enough to really maintain it.

tschaume added a commit to tschaume/toasted-marshmallow that referenced this pull request Jun 18, 2020
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.

2 participants