Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Work in progress] Marshmallow 3 port #188

Closed
wants to merge 4 commits into from
Closed

[Work in progress] Marshmallow 3 port #188

wants to merge 4 commits into from

Conversation

lafrech
Copy link
Member

@lafrech lafrech commented Jan 21, 2018

I've initiated a Marshmallow 3 port. Feedback welcome.

I'm not sure how to let tox/Travis test on both Marshmallow versions.

Regarding test_error_handler_is_called_regardless_of_schema_strict_setting, see #187.

@sloria
Copy link
Member

sloria commented Jan 22, 2018

Thanks so much for working on this!

I'll take a closer look at this when I'm back from my vacation (I'm out of my home country until Saturday).

@sloria
Copy link
Member

sloria commented Jan 22, 2018

I'm not sure how to let tox/Travis test on both Marshmallow versions.

Not sure about tox, but here's what we do for marshmallow-sqlalchemy on travis: https://github.com/marshmallow-code/marshmallow-sqlalchemy/blob/dev/.travis.yml

webargs/core.py Outdated
@@ -477,6 +484,9 @@ def location_handler(self, name):

@parser.location_handler('name')
def parse_data(request, name, field):
# Marshmallow 3
Copy link
Member

Choose a reason for hiding this comment

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

Drive-by comment: request here is a hypothetical Request object, not a MarshalResult, so no need to change the docs here

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right. Fixed. (I'll squash all commits when done.)

@lafrech
Copy link
Member Author

lafrech commented Jan 31, 2018

From #184:

I'm torn about keeping MA2-compatibility in webargs 3. I think if we do keep MA2-compat, we wouldn't need to bother supporting two release lines in webargs.
Do you have a preference either way, @lafrech ?

I believe it is much better if we can support both MA versions in a single webargs branch. Maintaining two webargs branches would be a heavier burden IMHO.

This is what this PR shows. The double-MA support is only a few lines of code in the lib. Most of the overhead is in the tests, and it is quite redundant but not that complex. Also it is all easy to spot and remove when MA2 is dropped. This is my favourite option.

I'm pretty sure we can do just the same with apispec.

If we agree on that single-branch principle, then I let you decide whether we keep a dev_marshmallow_3 branch here until MA 3.0.0 is released (in which case I can send a new PR against that branch) or if we merge this in dev right away. Or maybe we can just keep this PR open until MA3 and update it as changes are introduced in marshmallow.

A dedicated branch could be the cleanest way, but we'll have to merge in there all the changes from dev. For now, we might as well keep this open, and I'll rebase when needed. Let's not overthink this. It's not as if the MA2->MA3 change had such a great impact.

@sloria
Copy link
Member

sloria commented Feb 2, 2018

I agree--supporting both marshmallow 2 and 3 would be preferable, and it looks like doing so won't add too much complexity to the codebase.

Once you have this is in a working state, I can create a marshmallow-3 branch from it.

@lafrech
Copy link
Member Author

lafrech commented Feb 2, 2018

OK, great.

The failing test is linked to #187. I'm not sure what to do about it.

Apart from this, if you think this implementation is fine, then I shall

  • remove that print statement I forgot
  • squash
  • rebase

@sloria
Copy link
Member

sloria commented Feb 3, 2018

@lafrech Implementation looks good to me.

I've removed that pesky test. Will you take care of updating .travis.yml to test against both marshmallow 2 and 3?

@sloria
Copy link
Member

sloria commented Feb 3, 2018

Erm, nvm; we already test against marshmallow 3 pre-releases. I've added

matrix:
  allow_failures:
    - env: MARSHMALLOW_VERSION=""

...so that marshmallow releases won't break webargs builds in the meantime. I suppose you can remove that in this PR.

@lafrech
Copy link
Member Author

lafrech commented Feb 3, 2018

Great.

One more detail. I just noticed this in the tests:

MARSHMALLOW_VERSION_INFO = tuple(
    [int(part) for part in marshmallow.__version__.split('.') if part.isdigit()]
)

Do you think I should use something like this in the code rather than

    MARSHMALLOW_2 = ma.__version__.startswith('2')

?

I've been searching for good ways of determining if the version is 2 or 3 from a semver string but I didn't find anything obvious and I thought it would be a shame to add a dedicated lib just to do that. So I settled with that simple startswith which should work in all cases.

I could move

MARSHMALLOW_VERSION_INFO = tuple(
    [int(part) for part in marshmallow.__version__.split('.') if part.isdigit()]
)

to the lib code (rather than the tests) and use MARSHMALLOW_VERSION_INFO[0] to know the version.

@sloria
Copy link
Member

sloria commented Feb 3, 2018

Yeah, I think it makes sense to move MARSHMALLOW_VERSION_INFO to the lib code and use that everywhere.

@lafrech
Copy link
Member Author

lafrech commented Feb 3, 2018

Alright. I just pushed marshmallow-3 branch.

@lafrech lafrech closed this Feb 3, 2018
@sloria
Copy link
Member

sloria commented Feb 7, 2018

Went ahead and merged marshmallow-3 into dev. Since it's backwards-compatible, this can go into the 1.10.0 release.

@lafrech
Copy link
Member Author

lafrech commented Feb 7, 2018

OK.

Should we reallow failures for MA3 until v3 stable ships?

@sloria
Copy link
Member

sloria commented Feb 7, 2018

@lafrech I think we can keep dev up to date with the latest MA3 release, in which case we shouldn't allow failures.

@lafrech
Copy link
Member Author

lafrech commented Feb 7, 2018

Yeah, right. It's not like MA3 is moving that fast.

@lafrech lafrech deleted the dev_marshmallow_3 branch July 5, 2018 07:25
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.

None yet

2 participants