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

Replication #26

Merged
merged 10 commits into from Feb 1, 2016
Merged

Replication #26

merged 10 commits into from Feb 1, 2016

Conversation

almet
Copy link
Member

@almet almet commented Nov 30, 2015

That's now ready for review!

@almet
Copy link
Member Author

almet commented Feb 1, 2016

r? @leplatrem

return self._paginated(next_page, records,
with_headers=with_headers,
if_none_match=if_none_match)
if with_headers is True:
Copy link
Member Author

Choose a reason for hiding this comment

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

Not really sure about this one: it's a way to return the headers, as they can be useful sometimes.

Copy link
Contributor

Choose a reason for hiding this comment

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

hum, yes but that is strange to alter the return type based on input parameter

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 removed it, it seems not to be used anymore in the kinto-updater.

@almet
Copy link
Member Author

almet commented Feb 1, 2016

This PR introduces a regression in the kinto-updater. I'm investigating which tests are missing here :)

request = {
'method': method.upper(),
'path': url}
'path': url.replace('v1/', '')}
Copy link
Contributor

Choose a reason for hiding this comment

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

do you have a test somewhere for this one?

@leplatrem
Copy link
Contributor

I like it globally, except the with_headers thing. I'm not able to suggest any alternative right away though :(

Otherwise the replication doesn't work properly and new last_modified values
are created for each record, whereas we need to have the same one between
servers.
It's not used anymore in the clients and makes the API a bit weird by returning
a different thing (not the same number of arguments) depending on parameters.
- Better wording for the replication function.
- Add default values for exceptions.response and exception.request
- Add tests for utils.quote()
- Improve readability of some function.
@leplatrem
Copy link
Contributor

r+ !

almet added a commit that referenced this pull request Feb 1, 2016
@almet almet merged commit 4518bfe into master Feb 1, 2016
@almet almet deleted the replication branch February 1, 2016 21:11
@almet almet removed the in progress label Feb 1, 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