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 passing of kwargs to init_app #722

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jethroguce
Copy link

initiating app instance with arguments does not take effect,

api = Api(app, add_specs=False)

but when using the lazy instantiation it works

api = Api()
api.init_app(app, add_specs=False)

@coveralls
Copy link

coveralls commented Oct 2, 2019

Coverage Status

Coverage decreased (-0.07%) to 96.837% when pulling 888b9ae on jethroguce:master into a7e363a on noirbizarre:master.

@jethroguce
Copy link
Author

@j5awry can you have a quick look at this

@j5awry
Copy link
Collaborator

j5awry commented Oct 4, 2019

  1. we should check the tests for coverage of this new call
  2. looking at the history, git blame, and the commented portion, it looks like this capability existed before. Looks like 4 years ago there was a switch, and the old way did pass kwargs to a parent class (that looks like it did do what this is asking for)

From that standpoint, it's worth asking if this is meant to, since it seems to be a conscious change. I'm in favor of passing kwargs down though.

@SteadBytes
Copy link
Collaborator

I'm also in favour of passing kwargs down, however it would certainly be useful to understand the original reason for removing it if possible.

2 things before merging if you don't mind:

  1. Cover the change with tests
  2. Remove https://github.com/noirbizarre/flask-restplus/pull/722/files#diff-31928362e95b5b97b24d2ccf01506003R153 - may as well clean it up a bit whilst here 😄

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