Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Only use default parameter values for path parameters (#450)#464

Merged
tiangolo merged 17 commits intofastapi:masterfrom
jonathanunderwood:infer_optional_params
Sep 29, 2019
Merged

Only use default parameter values for path parameters (#450)#464
tiangolo merged 17 commits intofastapi:masterfrom
jonathanunderwood:infer_optional_params

Conversation

@jonathanunderwood
Copy link
Contributor

This change allows endpoint functions to be defined with parameters with defaults, but for those defaults to be ignored if the function parameter subsequently appears as a path parameter. The default value is only operative if the parameter is a query parameter.

This change includes a new set of tests for this functionality.

@jonathanunderwood
Copy link
ContributorAuthor

WIP: this approach is too aggressive as it ignores allPath() specifications. Will fix later.

@codecov
Copy link

codecovbot commentedAug 26, 2019

Codecov Report

Merging#464 intomaster willdecrease coverage by0.01%.
The diff coverage is100%.

Impacted file tree graph

@@            Coverage Diff             @@##           master     #464      +/-   ##==========================================- Coverage     100%   99.98%   -0.02%==========================================  Files         242      243       +1       Lines        5677     5746      +69     ==========================================+ Hits         5677     5745      +68- Misses          0        1       +1
Impacted FilesCoverage Δ
fastapi/dependencies/utils.py99.67% <100%> (-0.33%)⬇️
tests/test_infer_param_optionality.py100% <100%> (ø)

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updateb77a43b...3ee359e. Read thecomment docs.

@codecov
Copy link

codecovbot commentedAug 26, 2019•
edited
Loading

Codecov Report

Merging#464 intomaster willnot change coverage.
The diff coverage is100%.

Impacted file tree graph

@@          Coverage Diff           @@##           master   #464    +/-   ##======================================  Coverage     100%   100%            ======================================  Files         253    243    -10       Lines        6149   5749   -400     ======================================- Hits         6149   5749   -400
Impacted FilesCoverage Δ
fastapi/dependencies/utils.py100% <100%> (ø)⬆️
tests/test_infer_param_optionality.py100% <100%> (ø)
tests/test_additional_responses_router.py100% <0%> (ø)⬆️
fastapi/openapi/docs.py100% <0%> (ø)⬆️
...test_tutorial/test_path_params/test_tutorial005.py100% <0%> (ø)⬆️
fastapi/openapi/utils.py100% <0%> (ø)⬆️
tests/test_local_docs.py100% <0%> (ø)⬆️
fastapi/applications.py100% <0%> (ø)⬆️
fastapi/encoders.py100% <0%> (ø)⬆️
... and18 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updateb9cf69c...864c6ae. Read thecomment docs.

@jonathanunderwoodjonathanunderwoodforce-pushed theinfer_optional_params branch 2 times, most recently from9780900 to14429a2CompareAugust 26, 2019 07:12
@jonathanunderwood
Copy link
ContributorAuthor

jonathanunderwood commentedAug 26, 2019•
edited
Loading

As far as I can see this should be mergeable. The travis runs are raising errors with incorrectly sorted imports in masses of other files unrelated to this commit, so I'm not sure what's going on there.@tiangolo please advise.

@euri10
Copy link
Contributor

euri10 commentedAug 26, 2019 via email

Install locally with flit as described in the docLe lun. 26 août 2019 à 9:13 AM, jonathanunderwood <notifications@github.com>a écrit :
…
As far as I can see this should be mergeable. The travis runs are raising errors with incorrectly sorted imports in masses of other files unrelated to this commit, so I'm not sure what's going on there. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#464>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAINSPUAYBDIVXS5M2Z4QFLQGN7BNANCNFSM4IPI77SQ> .
jonathanunderwood reacted with thumbs up emoji

@jonathanunderwood
Copy link
ContributorAuthor

OK, fixed the import order issue. All should be good now.

@euri10
Copy link
Contributor

OK, fixed the import order issue. All should be good now.

been bitten by it before too ;)#38 (comment)

jonathanunderwood reacted with laugh emoji

@jonathanunderwood
Copy link
ContributorAuthor

@tiangolo any feedback on this one?

Copy link
Member

@tiangolotiangolo left a comment

Choose a reason for hiding this comment

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

Good job! Thanks for your contribution! 🚀 🍰 👏

I hadn't thought about this use case but I see how it could be useful in those cases.

LGTM in general 🎉

Just a couple of nitpick formatting requests, I inlined them all, so you just have to "accept" them. Sorry for the many suggestions, GitHub doesn't let me do multiline suggestions 😬

jonathanunderwood reacted with thumbs up emoji
@tiangolo
Copy link
Member

Thanks for the help here@euri10 ! 🙇‍♂️

@dmontagu if you have time, could you give this a quick review?

@jonathanunderwood
Copy link
ContributorAuthor

All suggested formatting changes have been added.

@jonathanunderwood
Copy link
ContributorAuthor

MyPy tests are now failing in code unrelated to this change, which is strange.

This change allows endpoint functions to be defined with parameterswith defaults, but for those defaults to be ignored if the functionparameter subsequently appears as a path parameter. The defaultvalue is only operative if the parameter is a query parameter.This change includes a new set of tests for this functionality.
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
Co-Authored-By: Sebastián Ramírez <tiangolo@gmail.com>
@tiangolo
Copy link
Member

Thanks for the format changes!

Yeah, the samemypy error just happened in a couple of other PRs. It's related to some changes in a new version ofmypy. I just rebased your branch from master, as it has those fixes (from these recent PRs). Let's see if that fixes it.

Copy link
Member

@tiangolotiangolo left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉 🍰

@tiangolotiangolo merged commitb20b221 intofastapi:masterSep 29, 2019
@tiangolo
Copy link
Member

Thanks for your contribution@jonathanunderwood ! 🍰 🎉 🚀

jonathanunderwood reacted with thumbs up emoji

@jonathanunderwood
Copy link
ContributorAuthor

Thanks for taking the time to review this change, much appreciated.

tiangolo reacted with rocket emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@tiangolotiangolotiangolo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jonathanunderwood@euri10@tiangolo

Comments

[8]ページ先頭
©2009-2026 Movatter.jp