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

Update json for Flask 2.3 #1582

Merged
merged 2 commits into from
Sep 6, 2022
Merged

Update json for Flask 2.3 #1582

merged 2 commits into from
Sep 6, 2022

Conversation

RobbeSneyders
Copy link
Member

Partially fixes #1576 by updating the jsonifier and related code to be compatible with Flask 2.3.
Since this new json interface in Flask was only introduced in Flask 2.2, we'll have to pin it as the lower bound version.

I'm doubting if we should backport this to v2. It introduces some breaking changes for users who use custom json encoders, and it would force us to bump the lower bound version of Flask to 2.2 there as well.

If we don't do a backport, we should pin the upper bound Flask version on v2 to 2.2 instead.

@coveralls
Copy link

coveralls commented Sep 3, 2022

Pull Request Test Coverage Report for Build 2985812338

  • 17 of 19 (89.47%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 94.731%

Changes Missing Coverage Covered Lines Changed/Added Lines %
connexion/apps/flask_app.py 5 6 83.33%
connexion/jsonifier.py 12 13 92.31%
Totals Coverage Status
Change from base Build 2957760283: 0.07%
Covered Lines: 2715
Relevant Lines: 2866

💛 - Coveralls

Copy link
Collaborator

@nielsbox nielsbox left a comment

Choose a reason for hiding this comment

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

LGTM
we should pin the upper bound Flask version on v2 to 2.2 instead.
I agree with not backporting and pinning the version instead. Seems like an unnecessary breaking change we do not wish to introduce in a minor version increase from a Connexion perspective.

On a side note: Are there features from 2.2> that some users cannot wait for? Maybe an unofficial release could be the answer for that?

@RobbeSneyders
Copy link
Member Author

On a side note: Are there features from 2.2> that some users cannot wait for?

2.2 is currently the latest version, so hard to say. Biggest risk is that it will still take a while before we can release 3.X, and we'll be lagging multiple versions behind Flask. Of course we can still choose to backport this later on.

Maybe an unofficial release could be the answer for that?

What do you mean with an unofficial release? I wouldn't like to branch our releases even further. We now have 2 branches to maintain (3.X and 2.X) which already requires some additional effort. I wouldn't want to have to support a 3rd :)

@nielsbox
Copy link
Collaborator

nielsbox commented Sep 6, 2022

On a side note: Are there features from 2.2> that some users cannot wait for?

2.2 is currently the latest version, so hard to say. Biggest risk is that it will still take a while before we can release 3.X, and we'll be lagging multiple versions behind Flask. Of course we can still choose to backport this later on.

Exactly my thoughts. Since we are currently developing 3.0 we cannot issue a new breaking release (we can from a separate branch, but I don't think it is worth it IMO like you said)

Maybe an unofficial release could be the answer for that?

What do you mean with an unofficial release? I wouldn't like to branch our releases even further. We now have 2 branches to maintain (3.X and 2.X) which already requires some additional effort. I wouldn't want to have to support a 3rd :)

Yeah, I meant exactly that but looking back on my suggestion it seems a bit too much, users will be expecting support and maintenance. So let's just pin it and keep it at that. We can always backport this later on, indeed.

@RobbeSneyders
Copy link
Member Author

Ok, I'll submit a PR to the v2 branch to limit the upper version of Flask.

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.

Pytest generates 2,464 warnings that should be investigated
3 participants