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

支持仅配置需自定义的配置项 #383

Closed
wants to merge 1 commit into from

Conversation

luohuaizhi
Copy link

@luohuaizhi luohuaizhi commented Apr 11, 2020

Allow partial config to be merged with default config 支持仅配置需自定义的配置项

@billyrrr
Copy link
Member

这个方案可能会对新用户造成混淆,在末尾新建一个’merge_config’ kwarg 是不是更好一些?或者其他名字也可以

This may cause confusion in new users. How about adding an additional ‘merge_config’ kwarg at the end of the list? You may also come up with a better name.

@luohuaizhi
Copy link
Author

这个方案可能会对新用户造成混淆,在末尾新建一个’merge_config’ kwarg 是不是更好一些?或者其他名字也可以

This may cause confusion in new users. How about adding an additional ‘merge_config’ kwarg at the end of the list? You may also come up with a better name.

新用户使用默认配置没有影响,这是进阶用户才会使用到的功能

@joeystevens00
Copy link
Contributor

I find the existing functionality confusing and this PR to be an improvement.

This should be valid but it's not:

swagger = Swagger(app,  config={
    "swagger_ui": False,
})

Instead users have to provide something like

swagger = Swagger(app,  config={
    "headers": [
    ],
    "specs": [
        {
            "endpoint": 'apispec_1',
            "route": '/apispec_1.json',
            "rule_filter": lambda rule: True,  # all in
            "model_filter": lambda tag: True,  # all in
        }
    ],
    "static_url_path": "/flasgger_static",
    "swagger_ui": False,
})

@billyrrr
Copy link
Member

billyrrr commented Apr 20, 2020

Since we have existing users expecting config to entirely replace self.DEFAULT_CONFIG.copy(), this change have the potential to create unexpected behaviors. It is nevertheless true that current behavior can be confusing. I think that a pull request with a different field name for this use case would be helpful. For this keyword argument, it is better to default to None and be initialized in function body to a dictionary out of convention. Additionally, a warning may be added in the documentations to warn the users that config replaces rather than amends self.DEFAULT_CONFIG.copy(). What do you think? @luohuaizhi @joeystevens00

由于我们已有用户希望config能够完全替代self.DEFAULT_CONFIG.copy(),因此此更改有可能产生意外行为。 但是,当前的行为确实令人困惑。 我认为对于此用例,使用不同字段名称的PR会有所帮助。 对于这个关键字参数,最好默认为None并在函数体中初始化为dict()以此符合约定。 另外,最好在文档中添加警告以警告用户config代替而不是修改self.DEFAULT_CONFIG.copy()。你怎么看?

@billyrrr
Copy link
Member

A "merge=False" keyword argument will be added to the __init__ argument list and, when "merge=True", the behavior will be as defined in this commit. I will rebase and merge this commit with the above modification soon.

@billyrrr
Copy link
Member

Merged in 63ed0ca with modifications. Thank you for your contribution!

@billyrrr billyrrr closed this May 11, 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

3 participants