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

Add ndb support. #40

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add ndb support. #40

wants to merge 6 commits into from

Conversation

unprolix
Copy link

@unprolix unprolix commented Mar 8, 2013

Thanks for the useful code!

I've made this modification to add ndb support; feel free to take it if you like.

@mdornseif
Copy link
Contributor

That's a bunch of different changes. Some seem to break existing sessions. May I suggest breaking them down into separate pull requests? 6fced82 seems to be the most obvious candidate for that.

@unprolix
Copy link
Author

I'm not sure what you see, but I don't see a reasonable way to break
that pr into multiples; it's fairly straightforward and actually
shouldn't even break old-style sessions. Obviously discard if you like!

On 9/24/13 6:31 PM, Maximillian Dornseif wrote:

That's a bunch of different changes. Some seem to break existing
sessions. May I suggest breaking them down into separate pull
requests? 6fced82
6fced82 seems to be the
most obvious candidate for that.


Reply to this email directly or view it on GitHub
#40 (comment).

@mdornseif
Copy link
Contributor

I mean that this actually consists of

  1. allow storing of ndb Objects in Sessions 6fced82
  2. switches to ndb for session storage d3fdc17
  3. allow to support old db backed sessions 878c41b - closely related to 2.
  4. adds some (experimental?) undocumented Cookie code 81565bc

I think breaking these down into different pull requests makes it much easier for the project owner (@dound) to evaluate merge them. From my personal experience I can say that the shorter the pull request the easier it get's accepted.

@unprolix
Copy link
Author

This is a reasonable analysis but at the same time, the changes are still straightforward and I have close to no investment in them being picked up by anyone else, so if the original developer is interested, he or she can incorporate some or all of them or not!

@dcoleyoung
Copy link

Just coming across this now. In the file there are some needed updates for DIRTY_BUT_DONT_PERSIST_TO_DB => DIRTY_BUT_DONT_PERSIST_xTO_DB

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