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

Add an upgrading doc to help users switch to 6.0 #489

Merged
merged 2 commits into from
Apr 11, 2020

Conversation

sirosen
Copy link
Collaborator

@sirosen sirosen commented Mar 17, 2020

The doc covers several issues, and explicitly is meant to address and resolve #485.

The doc is modeled on the marshmallow upgrading doc, and documents every backwards incompatible change which was made in 6.0, along with an example of impacted code and a rewrite which restores equivalent or near-equivalent behavior.

The doc mostly steers clear of trying to explain the decisions made, but it does provide some context, especially at the beginning when we have to show major signature changes in use_args/use_kwargs/parse.
Most of the suggested rewrites are simple, but a couple -- most notably the UnpackingDelimitedListSchema case -- are ideas which might be non-obvious to users.

Except when documenting specific changes to parse and related functions, everything is using locations=("query",) to specify "query params only" in the 5.x version of the code. That way, the translation to 6.x remains very simple in these cases.

@lafrech, I remember you saying when we did the 6.0 release that you were going to skip on writing an upgrade guide, since the changelog is pretty comprehensive. I agreed, but after seeing #483 and #485 , I figured I'd take a couple of hours and knock this out.

The doc covers several issues, and explicitly is meant to address and
resolve marshmallow-code#485.

The doc is modeled on the marshmallow upgrading doc, and documents
every backwards incompatible change which was made in 6.0, along with
an example of impacted code and a rewrite which restores equivalent or
near-equivalent behavior.

The doc mostly steers clear of trying to explain the decisions made,
but it does provide some context, especially at the beginning when we
have to show major signature changes in
`use_args`/`use_kwargs`/`parse`.
Most of the suggested rewrites are simple, but a couple -- most
notably the UnpackingDelimitedListSchema case -- are ideas which might
be non-obvious to users.

Except when documenting specific changes to `parse` and related
functions, everything is using `locations=("query",)` to specify "query
params only" in the 5.x version of the code. That way, the translation
to 6.x remains very simple in these cases.
docs/upgrading.rst Outdated Show resolved Hide resolved
Copy link
Member

@lafrech lafrech left a comment

Choose a reason for hiding this comment

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

Thank you so much for this.

I made a few comments. Otherwise, good to go!

Also, I generally prefer to avoid addressing the reader directly, using "you", using "one" instead. Merely a matter of style and I don't really mind. This doc is too precious to block on this.

docs/upgrading.rst Outdated Show resolved Hide resolved
docs/upgrading.rst Outdated Show resolved Hide resolved
docs/upgrading.rst Outdated Show resolved Hide resolved
docs/upgrading.rst Outdated Show resolved Hide resolved
docs/upgrading.rst Show resolved Hide resolved
@sirosen
Copy link
Collaborator Author

sirosen commented Mar 23, 2020

I like all of your rewording. I'll do all of these except for the pyramid one right now -- I want to check the behavior and make sure I have a correct example of the change.

Mostly per review from @lafrech, but this also fixes the pyramid
parser change doc.
`request` is always passed first, but other argument ordering is
inverted in `@use_args`.

Additionally, this fixes all of the `use_args` calls to have less
confusing parameter names. Because it passes the args as a dict, use
the name `args` and show it can be accessed with `args.get("q")` in
each case. Naming the arg dict `q` after the only field used could
suggest to users that `use_args` passes fields individually in some
way.
@sirosen
Copy link
Collaborator Author

sirosen commented Mar 23, 2020

I've just gotten to testing the PyramidParser behaviors and pushing the fix. I gave a new example in which there's a user-defined decorator passing additional data, so it clearly shows how use_args changes ordering.

I also noticed that the use_args calls in this doc were using a kind of confusing convention of naming the arg dict after the only field used, like

@use_args({"q": fields.String()})
def foo(q):
    bar(q)

To make the behavior of use_args less confusing, I changed all of these to

@use_args({"q": fields.String()})
def foo(args):
    bar(args.get("q"))

@lafrech lafrech merged commit 616fa2e into marshmallow-code:dev Apr 11, 2020
@lafrech
Copy link
Member

lafrech commented Apr 11, 2020

Sorry, I forgot about this.

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.

6.0.0 requires changes to some pre_load hooks
2 participants