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

Per @jimfulton, handle_all_serials shouldn't be sniffing in ZODB5 #87

Merged
merged 3 commits into from Jul 12, 2016
Merged

Per @jimfulton, handle_all_serials shouldn't be sniffing in ZODB5 #87

merged 3 commits into from Jul 12, 2016

Conversation

jamadden
Copy link
Member

@jamadden jamadden commented Jul 9, 2016

So try that. This slightly simplifies _dostore.

There are lots of other callers still so I didn't remove the functions entirely (but I could try if desired).

@jamadden
Copy link
Member Author

Turns out that at least the tests pass sequences of non-bytes to this method on Python 3 (probably str?), so I had to remove that assert.

@@ -194,10 +178,7 @@ def _dostore(self, oid=None, revid=None, data=None,
r1 = self._storage.store(oid, revid, data, '', t)
Copy link
Member

Choose a reason for hiding this comment

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

We don't use r1 and r2, so why assign them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to make the diff as minimal as possible. I can clean that up.

@jimfulton
Copy link
Member

Gutting the methods is confusing. Let's not leave this for a future code reader to stumble on. (It will probably be me and I just hurt myself for the last few minutes on this :) ).

We should remove these functions and their call points, or just remove the calls in dostore, if that's what was hurting RelStorage.

@jamadden
Copy link
Member Author

On Jul 12, 2016, at 11:00, Jim Fulton notifications@github.com wrote:

Gutting the methods is confusing. Let's not leave this for a future code reader to stumble on. (It will probably be me and I just hurt myself for the last few minutes on this :) ).

We should remove these functions and their call points, or just remove the calls in dostore, if that's what was hurting RelStorage.

OK, I can remove the functions and their callers. That will involve touching three more files, but I think it's mostly copy-pasted code.

@jamadden
Copy link
Member Author

Travis appears to be green with the removal. I say appears because github has been lagging a lot lately.

@jimfulton jimfulton merged commit 686f169 into zopefoundation:master Jul 12, 2016
@jmuchemb jmuchemb mentioned this pull request Jul 18, 2016
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