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

fix: rename incorrect keyword argument to parse_query_string #449

Closed

Conversation

dodumosu
Copy link

falconparser uses falcon.util.uri.parse_query_string, but uses
an incorrect keyword argument name, keep_blank_qs_values. the correct
argument name is keep_blank

see: #448

`falconparser` uses `falcon.util.uri.parse_query_string`, but uses
an incorrect keyword argument name, `keep_blank_qs_values`. the correct
argument name is `keep_blank`

see: marshmallow-code#448
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.

Good.

I think it makes sense to maintain compatibility with Falcon 1. Flacon 2 is really recent and backward compat is cheap.

Please also add yourself to AUTHORS.

src/webargs/falconparser.py Show resolved Hide resolved
src/webargs/falconparser.py Outdated Show resolved Hide resolved
src/webargs/falconparser.py Show resolved Hide resolved
this commit mostly consists of stylistic changes, for example
using `distutils.version.LooseVersion` instead of the raw Falcon
version string.

other than that, the major change is dealing all Falcon versions
greater than 1.X as using the updated keyword arg, and Falcon 1.x
versions as using the legacy keyword arg (as opposed to just
Falcon 1.x and 2.x).
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.

Great. Just one typo to fix.

src/webargs/falconparser.py 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.

One last change I didn't think of before.

Thanks.

src/webargs/falconparser.py Outdated Show resolved Hide resolved
@lafrech
Copy link
Member

lafrech commented Dec 24, 2019

If we mean to support both Flacon 1 and 2, we should ensure the tests test both version. @dodumosu if you're not comfortable with modifying CI tests config, we'll do it when we get the time.

Is Flacon 1 still maintained? Shall we drop support for it in the next webargs major version ?

@Nateyo
Copy link
Contributor

Nateyo commented Dec 31, 2019

As far as I know, most of the development going on with falcon is now 2.0+. This PR should likely update the setup.py as well since right now it specifies falcon>=1.4.0,<2.0. There's actually another problem with args to req.stream.read() which I can submit a PR for, building on top of @dodumosu's changes

@lafrech
Copy link
Member

lafrech commented Dec 31, 2019

What about keeping webargs 5.x Falcon 1.x and dropping Falcon 1.x in webargs 6.0?

webargs Falcon
5 1 only
6 2 only

@lafrech
Copy link
Member

lafrech commented Dec 31, 2019

@sloria, we could

We could publish beta releases along the way.

@sloria
Copy link
Member

sloria commented Jan 2, 2020

@lafrech I'm good with that plan

@lafrech
Copy link
Member

lafrech commented Jan 3, 2020

Guys, I just merged #420.

You may rebase this and drop Falcon 1 (or start from scratch if it is easier), and send another PR to fix req.stream.read().

Thanks.

@lafrech
Copy link
Member

lafrech commented Jan 6, 2020

Better wait until #454 is merged to avoid merge conflicts. Done.

@lafrech
Copy link
Member

lafrech commented Jan 10, 2020

@dodumosu @Nateyo are you willing to tackle this?

No pressure, just asking. I could rebase the change in this PR when I get the time, but I don't know what @Nateyo has in mind regarding req.stream.read() and I don't use Falcon so ideally I'd rather defer that to you.

Basically, change setup.py to require falcon>=2 and do all required changes (the two expressed above + others if needed).

@Nateyo
Copy link
Contributor

Nateyo commented Jan 12, 2020

@lafrech Sure, I can work on it this week. I can cherry pick these commits and submit a whole new PR to the dev branch? Would that be easier since you want 5.x to keep Falcon 1 compat?

@lafrech
Copy link
Member

lafrech commented Jan 12, 2020

Great. Thanks @Nateyo.

Further dev on webargs 5 will take place in branch 5.x-line. We're dropping Falcon 1 in dev branch. I wouldn't cherry-pick and just start from scratch as the changeset is much smaller when dropping Falcon 2 than when keeping compatibility. You may submit a single PR with the whole Falcon 2 move.

@lafrech lafrech added this to the 6.0.0 milestone Jan 12, 2020
@lafrech
Copy link
Member

lafrech commented Jan 21, 2020

Superseded by #459.

@lafrech lafrech closed this Jan 21, 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.

None yet

4 participants