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

adding abillity to define services imperative #335

Merged
merged 4 commits into from
Oct 11, 2015

Conversation

delijati
Copy link
Contributor

@delijati delijati commented Sep 9, 2015

See issue #334 for more info. Coverage for sphinxext increased to 60% :)

@@ -30,6 +30,24 @@ Here is how you can register a resource::
_USERS[len(_USERS) + 1] = self.request.json_body
return True

Here a example of imperative way defining a resource::
Copy link
Contributor

Choose a reason for hiding this comment

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

consider: "Here is an example of how to define cornice resources in an imperative way"

@almet
Copy link
Contributor

almet commented Sep 14, 2015

Hi!

Thanks for taking the time to submit this pull request. That's appreciated. I like how the decorators are now completely optional, and in some way, it closes the gap I started to fill when I did the cornice refactor.

I'm not a big fan of the add_resource and add_view names, but they seem to match with the ones provided by pyramid itself, so I think it's fine to let it like that.

I've added a few comments and I think we're good for merging once they're addressed.

adding ametaireau changes
extended sphinx tests for resource
@delijati
Copy link
Contributor Author

Hi,

i've added the directive add_cornice_resource to get rid of the scan. There is still that issue with ordering of the resource views. I think a cleaner way would be to add a Register Object like the Service object witch holds the collected informations. But this should be done in another refactoring ;)

@delijati
Copy link
Contributor Author

Do you had some time to look into that?

@delijati
Copy link
Contributor Author

delijati commented Oct 9, 2015

@leplatrem could you also take a look into that pull request

@almet
Copy link
Contributor

almet commented Oct 11, 2015

Hey Josip, this looks really good. Eveything is in there: impelmentation, docs, new tests, plus the old tests are passing.

Let's merge :-)

almet added a commit that referenced this pull request Oct 11, 2015
adding abillity to define services imperative
@almet almet merged commit 7f3a9b9 into Cornices:master Oct 11, 2015
@delijati
Copy link
Contributor Author

Wonderful do you know if someone is already working on the push request #285 ? If not i will look into that one next week.

@almet
Copy link
Contributor

almet commented Oct 11, 2015

Neat. Nobody is currently working on this I believe. Feel free to take it!

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.

2 participants