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

feat(firestore): add limit_to_last samples #4196

Merged
merged 5 commits into from
Jul 22, 2020

Conversation

IlyaFaer
Copy link

Closes #4195

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 29, 2020
@IlyaFaer IlyaFaer marked this pull request as ready for review June 29, 2020 08:33
@IlyaFaer IlyaFaer requested a review from a team as a code owner June 29, 2020 08:33
@IlyaFaer
Copy link
Author

IlyaFaer commented Jun 29, 2020

@BenWhitehead, @crwilcox, I've added a sample. Taken the tag name from the Java's samples

@BenWhitehead BenWhitehead added automerge Merge the pull request once unit tests and other checks pass. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jun 29, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 29, 2020
@gcf-merge-on-green
Copy link
Contributor

Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR

@BenWhitehead BenWhitehead removed the automerge Merge the pull request once unit tests and other checks pass. label Jun 29, 2020
@BenWhitehead
Copy link
Contributor

@IlyaFaer Looks like all three builds are failing with the following traceback

Traceback (most recent call last):
  File "/workspace/firestore/cloud-client/snippets_test.py", line 239, in test_order_limit_to_last
    snippets.order_limit_to_last()
  File "/workspace/firestore/cloud-client/snippets.py", line 579, in order_limit_to_last
    query = cities_ref.order_by("name").limit_to_last(2)
AttributeError: 'Query' object has no attribute 'limit_to_last'

@gcf-merge-on-green
Copy link
Contributor

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@IlyaFaer
Copy link
Author

@BenWhitehead, yeah, I suppose, it'll be failing until the next Firestore release. Locally I've replaced the Python original Firestore package with the last repository version, and it's working fine:

Безымянный

@tmatsuo tmatsuo added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 30, 2020
Copy link
Contributor

@tmatsuo tmatsuo left a comment

Choose a reason for hiding this comment

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

let me know when it's ready to merge

@@ -235,6 +235,10 @@ def test_order_where_limit():
snippets.order_where_limit()


def test_order_limit_to_last():
snippets.order_limit_to_last()
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to check the output?

Copy link
Contributor

Choose a reason for hiding this comment

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

@IlyaFaer Could you comment on this?

You can return the results in the function and check them here if that's easier than checking the string output.

results = snippets.order_limit_to_last()
assert ...

Copy link
Author

@IlyaFaer IlyaFaer Jul 9, 2020

Choose a reason for hiding this comment

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

Oh, that slipped my radars. Yeah, I thought about that, but most of the snippets tests are written this way - just a call to snippet function. So, as I see it, if we're gonna add an assert, we should do this for all the tests to keep the fashion, which probably should be done in a separate issue. Adding an assert into this single test, while other similar tests doesn't have it, looks weird. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create an issue for it?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, will make a /cc to you when created. And I think I'll do this

@busunkim96
Copy link
Contributor

If you want to check that CI passes, you can temporarily change the requirements.txt to install GitHub from the master branch.

https://github.com/q-logic/python-docs-samples/blob/limit_to_last_samples/firestore/cloud-client/requirements.txt#L1

git+https://github.com/googleapis/python-firestore.git

@IlyaFaer
Copy link
Author

@busunkim96, sounds interesting, thanks!
@tmatsuo, could you run kokoro checks? I don't have rights to add labels 😞
I've tried to change requirements.txt as adviced, let's give it a try

@BenWhitehead BenWhitehead added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 30, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 30, 2020
@BenWhitehead
Copy link
Contributor

Kokoro builds triggered

@IlyaFaer
Copy link
Author

@BenWhitehead, thanks! Checks are green. Please, acknowledge, and I'll revert requirements.txt
I've pinged the question of adding me into GoogleCloudPlatform, so, hopefully, I'll get some abilities soon

@IlyaFaer IlyaFaer added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 8, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 8, 2020
@IlyaFaer
Copy link
Author

IlyaFaer commented Jul 8, 2020

New Firestore client released, I've reverted requirements.txt changes, and now it's working fine.
Dropping "do not merge" label

@IlyaFaer IlyaFaer removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 8, 2020
firestore/cloud-client/snippets.py Show resolved Hide resolved
@@ -235,6 +235,10 @@ def test_order_where_limit():
snippets.order_where_limit()


def test_order_limit_to_last():
snippets.order_limit_to_last()
Copy link
Contributor

Choose a reason for hiding this comment

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

@IlyaFaer Could you comment on this?

You can return the results in the function and check them here if that's easier than checking the string output.

results = snippets.order_limit_to_last()
assert ...

@IlyaFaer
Copy link
Author

Friendly ping

@tmatsuo tmatsuo added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 22, 2020
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 22, 2020
@tmatsuo tmatsuo merged commit b239bb3 into GoogleCloudPlatform:master Jul 22, 2020
@IlyaFaer IlyaFaer deleted the limit_to_last_samples branch July 23, 2020 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firestore: add samples for limit_to_last feature
6 participants