Skip to content

Add support for path level parameters#455

Merged
lafrech merged 8 commits intomarshmallow-code:devfrom
karec:dev
Jun 18, 2019
Merged

Add support for path level parameters#455
lafrech merged 8 commits intomarshmallow-code:devfrom
karec:dev

Conversation

@karec
Copy link
Copy Markdown
Contributor

@karec karec commented Jun 12, 2019

For #453

  • Create clean_parameters function to avoid duplicate code
  • Extracted get_ref from clean_operations to reuse it in
    clean_parameters
  • Added parameters argument to APISpec.path()
  • Sending parameters argument to plugin.path_helper as a kwargs,
    allowing plugins to update it
  • Added tests for global parameters feature
  • Added tests for duplicate parameters in operations
  • Added name to existing parameters in tests as required by OpenAPI
    specification
  • Now raising an APISpecError in case of duplicate parameters in
    path / operations

* Create `clean_parameters` function to avoid duplicate code
* Extracted `get_ref` from `clean_operations` to reuse it in
  `clean_parameters`
* Added `parameters` argument to `APISpec.path()`
* Sending `parameters` argument to `plugin.path_helper` as a kwargs,
  allowing plugins to update it
* Added tests for global parameters feature
* Added tests for duplicate parameters in operations
* Added name to existing parameters in tests as required by OpenAPI
  specification
* Now raising an `APISpecError` in case of duplicate parameters in
  path / operations
@karec karec changed the title Add support for path level parameters #453 Add support for path level parameters Jun 12, 2019
@lafrech
Copy link
Copy Markdown
Member

lafrech commented Jun 12, 2019

Looks pretty good, thanks.

Nitpick: Splitting the changes in two commits (one for the refactor and one for the new feature) would make it easier to review, but not big deal. It is a small changeset.

Comment thread src/apispec/core.py
Comment thread src/apispec/core.py Outdated
Comment thread tests/test_core.py
Comment thread tests/test_core.py Outdated
Comment thread tests/test_core.py
* Raise this new error instead of `APIError` when finding duplicates
  in parameters
* Update tests to check if that exception is raised instead of base
  `APISpecError`
* Split tests for duplicate parameters
* New exception `InvalidParameterError`
* Now raising `InvalidParameterError` on `clean_parameters`
* Added tests to check that correct exception is thrown
Comment thread tests/test_ext_marshmallow.py
Comment thread tests/test_core.py Outdated
Comment thread src/apispec/core.py
Comment thread src/apispec/core.py
Comment thread src/apispec/core.py Outdated
@lafrech
Copy link
Copy Markdown
Member

lafrech commented Jun 13, 2019

This PR adds an InvalidParameterError that is raised when a parameter misses name or in attribute.

This change could break applications that don't provide a name or location. Those are applications with broken documentation since name and location are required. But broken doc is not broken app.

Should we consider this a breaking change? Major releases are cheap, webargs is already 5.x, bumping apispec to 2.0 wouldn't be an issue.

@sloria, what do you think?

karec added 2 commits June 13, 2019 13:53
* Update `clean_parameters` to factorize loops
* Typo fix in `get_ref` docstring
* Mark `test_invalid_parameter` as parametrized
* Forced `required` value to `True` if parameter location is set to
  "path"
* Updated `test_parameter` according to this
Comment thread tests/test_core.py Outdated
Comment thread src/apispec/core.py
@lafrech
Copy link
Copy Markdown
Member

lafrech commented Jun 13, 2019

Also, the signature of BasePlugin.path_helper has changed. Since the former signature includes **kwargs, it may not qualify as a breaking change.

@lafrech
Copy link
Copy Markdown
Member

lafrech commented Jun 13, 2019

@karec, looks like you committed with two different email addresses, one of which is not connected to your GH account. I absolutely don't mind but if you wish to change that, better now than never.

@sloria
Copy link
Copy Markdown
Member

sloria commented Jun 13, 2019

Should we consider this a breaking change? Major releases are cheap, webargs is already 5.x, bumping apispec to 2.0 wouldn't be an issue.

Yes, I've no issue bumping to 2.0. I won't have time to review this PR for at least a week or so, so go ahead and handle this as you please @lafrech

* Updated docstring to include kwargs decription
* Updated `apispec.plugin.BasePlugin.path` docstring to include
  `parameters`
* Updated `writing_plugins.rst` to reflect parameters changes and
  include `kwargs` example
* Updated `test_core.TestPlugins.test_plugin_path_hlper_is_used` to
  include `parameters` argument
Comment thread docs/writing_plugins.rst Outdated
Comment thread docs/writing_plugins.rst Outdated
Comment thread docs/writing_plugins.rst Outdated
Comment thread src/apispec/plugin.py Outdated
Comment thread src/apispec/plugin.py Outdated
Comment thread docs/writing_plugins.rst Outdated
@lafrech lafrech merged commit 2f41368 into marshmallow-code:dev Jun 18, 2019
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.

3 participants