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

issue #172: spec/buggy Path/Operation handling #188

Merged
merged 3 commits into from
Feb 7, 2021

Conversation

lalmeras
Copy link
Contributor

@lalmeras lalmeras commented Feb 7, 2021

Current code expect Path object to contain only method/Operation
declaration. Path object may contain $ref, summary, description, servers
and parameters entries.

If available, this entries are default values to apply children
Operation.

This fix drops unused entries ($ref, summary, description, servers) and
merge parameters:

  • unicity based on name/in unicity
  • Operation value takes precedence

This fix allows to parse spec file attached with #172

Current code expect Path object to contain only method/Operation
declaration. Path object may contain $ref, summary, description, servers
and parameters entries.

If available, this entries are default values to apply children
Operation.

This fix drops unused entries ($ref, summary, description, servers) and
merge parameters:
* unicity based on name/in unicity
* Operation value takes precedence

This fix allows to parse spec file attached with httpie#172
@jkbrzt
Copy link
Member

jkbrzt commented Feb 7, 2021

Thanks, @lalmeras! Can you look into the failed test? It’s basically this command :

http-prompt \
    https://api.github.com \
    --spec \
    https://raw.githubusercontent.com/github/rest-api-description/main/descriptions/api.github.com/api.github.com.json

Parameter may be a ``{ '$ref': '...' }``. We need to use $ref, name, in
attributes for processing parameter list merging.
@lalmeras
Copy link
Contributor Author

lalmeras commented Feb 7, 2021

Here's a fix for the regression. I work on a test for the new feature.

@jkbrzt
Copy link
Member

jkbrzt commented Feb 7, 2021

@lalmeras perfect, thanks!

Path and methods parameters are merged to handle completion.

Merge process is fixed to allow completion when parameters are
configured only at path level.
@lalmeras
Copy link
Contributor Author

lalmeras commented Feb 7, 2021

Added test and PR fix.

Test basically checks that all expected parameters are here when we use a path-level-only parameters definition (this is where a fix was needed) and a path-method-level parameter definition.

It seems to me that the proposal is complete, and should not trigger other regressions.

@jkbrzt jkbrzt merged commit ac598fd into httpie:master Feb 7, 2021
@jkbrzt
Copy link
Member

jkbrzt commented Feb 7, 2021

Merged, thanks! 🥧

@lalmeras lalmeras deleted the issue-172-path-operation-parameters branch February 8, 2021 10:21
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

2 participants