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

Added MethodViewResolver #847

Merged
merged 20 commits into from
Feb 4, 2019
Merged

Added MethodViewResolver #847

merged 20 commits into from
Feb 4, 2019

Conversation

smn-snkl
Copy link
Contributor

@smn-snkl smn-snkl commented Jan 12, 2019

This PR proposes a solution for #359

By subclassing RestyResolver and modifying its resolve_function_from_operation_id method, it is now possible to use automatic routing functionality with Flask's MethodView together with MethodViewResolver.

Changes proposed in this pull request:

  • modified resolve_function_from_operation_id to return Flask's MethodView functions

By subclassing RestyResolver and modifying its `resolve_function_from_operation_id` method, it is now possible to use automatic routing functionality with Flask's MethodView together with MethodViewResolver.
@dtkav
Copy link
Collaborator

dtkav commented Jan 13, 2019

Thanks @smn-snkl !

I think the next steps are:

  1. add tests
  2. update docs
  3. add an example

Do you have bandwidth to take that on?

@Bengreen
Copy link

can I suggest a change on line 163

cls_path = "{}.{}View".format(module, path.title() if path.islower() else path)

@smn-snkl
Copy link
Contributor Author

@Bengreen can you explain the reasoning behind this? The reason I'm asking this is that I changed the implementation yesterday (and just committed it) to work with camelCase resources and still support the standard operationId keys for manual routing

@smn-snkl
Copy link
Contributor Author

@dtkav Don't have the resources right now. May be able to do it in a few days. If anyone else wants to take it on, I'd be happy as well

@Bengreen
Copy link

I am trying out some examples of how I use MethodView in past against this Resolver and trying to work out suggestions to allow it to work across standard ways of using MethodView.
Will try to post some examples based on pet store.

@smn-snkl
Copy link
Contributor Author

@Bengreen cool, thanks! Make sure to check the latest commit as I fixed some use cases as well

@Bengreen
Copy link

@smn-snkl I made a PR to your patch to add an example for MethodViewResolver.
Let me know if it works for you?

@smn-snkl
Copy link
Contributor Author

Just merged @Bengreen 's example into this PR. Thanks!

@spec-first spec-first deleted a comment Jan 15, 2019
@Bengreen
Copy link

Bengreen commented Jan 15, 2019

I just made a PR for @smn-snkl patch to add tests.
Tests are a mirror of RestyResolver.

@smn-snkl
Copy link
Contributor Author

Thanks @Bengreen. Just merged your tests into this PR as well. Have to admit though I didn't carefully review this last merge

@smn-snkl
Copy link
Contributor Author

@Bengreen : CI fails for latest Python version. Do you mind fixing the issue? I invited you to my fork. I think it's just the order of the imports.

@spec-first spec-first deleted a comment Jan 16, 2019
@Bengreen
Copy link

PR for this Merge with @smn-snkl to correct isort issues and correct example

@spec-first spec-first deleted a comment Jan 17, 2019
Ben Greene added 3 commits January 17, 2019 13:06
@spec-first spec-first deleted a comment Jan 17, 2019
@Bengreen
Copy link

Can we get a review from @dtkav

Copy link

@Bengreen Bengreen left a comment

Choose a reason for hiding this comment

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

Code has been cleaned up, examples added and tests added. It follows similar concept to RestyResolver.

@dtkav dtkav self-assigned this Jan 22, 2019
Copy link
Collaborator

@dtkav dtkav left a comment

Choose a reason for hiding this comment

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

Please update the docs here: https://github.com/zalando/connexion/blob/master/docs/routing.rst
Otherwise this is looking great. Nice work!

@Bengreen
Copy link

@dtkav Docs added.

@spec-first spec-first deleted a comment Jan 23, 2019
Copy link
Collaborator

@dtkav dtkav left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@dtkav dtkav requested a review from jmcs January 23, 2019 02:54
@dtkav dtkav added the ready label Jan 23, 2019
@Bengreen
Copy link

I made a merge of most recent master to this branch

@jmcs
Copy link
Contributor

jmcs commented Feb 4, 2019

👍

@jmcs jmcs merged commit 9286745 into spec-first:master Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants