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

Update signature to match docs. #1

Closed
wants to merge 3 commits into from
Closed

Conversation

jean
Copy link

@jean jean commented Oct 13, 2012

In https://github.com/bluedynamics/souper.plone/blob/master/README.rst we read >>> soup = get_soup(context, 'my_soup_id') and in https://github.com/bluedynamics/souper.plone/blob/master/src/souper/plone/locator.rst we read >>> get_soup(plone, 'mysoup') (sometimes).

@rnixx
Copy link
Member

rnixx commented Oct 14, 2012

hi, thx for the pull request. we should do the other way around, update the docs instead of changing the function signature, the package is already used at several places and it's not worth to break them.

@jean
Copy link
Author

jean commented Oct 14, 2012

Hi rnixx, thanks for being patient. Yes, normally I would update the docs, but the signature seems back to front to me, the docs are divided on the topic, and I've added a BBB fallback with deprecation message so old code shouldn't break.

That said, I would quite understand if you decline the get_soup change.

daf5708 is a simple fix though.

jensens added a commit that referenced this pull request Oct 14, 2012
@jensens
Copy link
Member

jensens commented Oct 14, 2012

b05e0db fixes daf5708 - but we keep signature, its all in production already and soupid is more important than context, well even if its the other way around this way than most plone calls (getToolByName, ...) - but this is also used outside of zope/plone (pyramid, ..., name it)

@jensens jensens closed this Oct 14, 2012
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