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

Filters should support properties with leading/trailing whitespaces in the name. #424

Closed
silvolu opened this issue Dec 17, 2014 · 5 comments · Fixed by #430
Closed

Filters should support properties with leading/trailing whitespaces in the name. #424

silvolu opened this issue Dec 17, 2014 · 5 comments · Fixed by #430
Assignees
Labels
api: datastore Issues related to the Datastore API. 🚨 This issue needs some love. triage me I really want to be triaged.

Comments

@silvolu
Copy link
Contributor

silvolu commented Dec 17, 2014

.filter('property with space =', 1) or .filter(' property with leading and trailing spaces =', 1) should result in the properties names not losing the trailing/leading whitespaces.
More context in #294 (comment)

@silvolu silvolu added api: datastore Issues related to the Datastore API. fix-asap labels Dec 17, 2014
@dhermes
Copy link
Contributor

dhermes commented Dec 17, 2014

I'm happy to support this but it seems like allowing developers to define properties with any spaces is asking for undefined or unexpected behavior. Adding leading and trailing spaces even more so.

ISTM there are two separate points / sides to take:

  • Just because the API supports it doesn't mean it's a best-practice. An idiomatic library should encourage best-practices.
  • This implementation in gcloud-python is intended to be very low-level and map the API exactly. Only power users will need it and higher level ORMs such as ndb built on top of gcloud-python are intended for typical users.

In my mind, the 2nd point is the more valid one, but we are not at all being clear about this in our development. In addition we are spending time making gcloud.datastore and it's modules idiomatic as if non-power users would be interacting with it.

I strongly prefer a future that involves running ndb on top of gcloud-python, but would like to have some sort of plan in place.

Luckily, if we do decide to implement this @Alfus comments in #294 carve out a good chunk of what needs to be done.

@Alfus
Copy link

Alfus commented Dec 17, 2014

I is possible to have it both ways with two different versions of the
filter function. One precise one:
filter(" name ", "op", "value") # the field name is taken as is
and one concise one:
filter("name op", "value") # the field name + op is parsed in a restricted
way

but there really should be a way to specify a filter on any supported
property name.

On Tue Dec 16 2014 at 8:13:10 PM Danny Hermes notifications@github.com
wrote:

I'm happy to support this but it seems like allowing developers to define
properties with any spaces is asking for undefined or unexpected
behavior. Adding leading and trailing spaces even more so.

ISTM there are two separate points / sides to take:

  • Just because the API supports it doesn't mean it's a best-practice.
    An idiomatic library should encourage best-practices.
  • This implementation in gcloud-python is intended to be very
    low-level and map the API exactly. Only power users will need it and higher
    level ORMs such as ndb built on top of gcloud-python are intended for
    typical users.

In my mind, the 2nd point is the more valid one, but we are not at all
being clear about this in our development. In addition we are spending time
making gcloud.datastore and it's modules idiomatic as if non-power users
would be interacting with it.

I strongly prefer a future that involves running ndb on top of
gcloud-python, but would like to have some sort of plan in place.

Luckily, if we do decide to implement this @Alfus
https://github.com/Alfus comments in #294
#294 carve out
a good chunk of what needs to be done.

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

@tseaver
Copy link
Contributor

tseaver commented Dec 17, 2014

I really don't understand the use case for the two-argument form:

derived = base.filter('prop name =', 'value)

versus the more natural / idomatic three-argument spelling:

derived = base.filter('prop name', '=', 'value)

As a bonus, the three-argument form makes the problem with leading / trailing (even embedded) spaces go away.

@silvolu
Copy link
Contributor Author

silvolu commented Dec 17, 2014

I'm for the three-argument form as well. @jgeewax? #294 (comment)

@dhermes dhermes self-assigned this Dec 17, 2014
@dhermes
Copy link
Contributor

dhermes commented Dec 17, 2014

FYI to all. I am working on this right now and changing the signature to

def filter(self, property_name, operation, value):

so that now

filtered_query = query.filter('age', '>', 50)

will be the way to call filter.

This makes it much simpler to support property names with any arbitrary kind of whitespace and also is a true mapping to the underlying proto (I mentioned this before). In light of my comment above, being a truer mapping is a good thing.

Please chime in if there are any issues or concerns.

dhermes added a commit to dhermes/google-cloud-python that referenced this issue Dec 17, 2014
Also adding a test for property names with spaces in them.

Fixes googleapis#424.
dhermes added a commit to dhermes/google-cloud-python that referenced this issue Dec 17, 2014
Also adding a test for property names with spaces in them.

Fixes googleapis#424.
dhermes added a commit to dhermes/google-cloud-python that referenced this issue Dec 17, 2014
Also adding a test for property names with spaces in them.

Fixes googleapis#424.
dhermes added a commit to dhermes/google-cloud-python that referenced this issue Dec 17, 2014
Also adding a test for property names with spaces in them.

Fixes googleapis#424.
@jgeewax jgeewax modified the milestone: Datastore Stable Jan 30, 2015
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Apr 7, 2020
atulep pushed a commit that referenced this issue Apr 6, 2023
…424)

Source-Link: googleapis/synthtool@8e55b32
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:c6c965a4bf40c19011b11f87dbc801a66d3a23fbc6704102be064ef31c51f1c3
atulep pushed a commit that referenced this issue Apr 18, 2023
…424)

Source-Link: googleapis/synthtool@8e55b32
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:c6c965a4bf40c19011b11f87dbc801a66d3a23fbc6704102be064ef31c51f1c3
parthea added a commit that referenced this issue Jun 4, 2023
* fix(deps): Require google-api-core >=1.34.0, >=2.11.0

fix: Drop usage of pkg_resources

fix: Fix timeout default values

docs(samples): Snippetgen should call await on the operation coroutine before calling result

PiperOrigin-RevId: 493260409

Source-Link: googleapis/googleapis@fea4387

Source-Link: googleapis/googleapis-gen@387b734
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiMzg3YjczNDRjNzUyOWVlNDRiZTg0ZTYxM2IxOWE4MjA1MDhjNjEyYiJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* add gapic_version.py

* fix(deps): require google-api-core>=1.34.0,>=2.11.0

* chore: use templated setup.py and owlbot.py

* update version in gapic_version.py

* work around generator bug

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Anthonios Partheniou <partheniou@google.com>
parthea pushed a commit that referenced this issue Jul 6, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this issue Aug 15, 2023
parthea pushed a commit that referenced this issue Sep 20, 2023
* chore: Update gapic-generator-python to v1.11.2

PiperOrigin-RevId: 546510849

Source-Link: googleapis/googleapis@736073a

Source-Link: googleapis/googleapis-gen@deb64e8
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZGViNjRlOGVjMTlkMTQxZTMxMDg5ZmU5MzJiM2E5OTdhZDU0MWM0ZCJ9

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea added a commit that referenced this issue Sep 22, 2023
Co-authored-by: Anthonios Partheniou <partheniou@google.com>
parthea pushed a commit that referenced this issue Sep 22, 2023
Source-Link: https://togithub.com/googleapis/synthtool/commit/30bd01b4ab78bf1b2a425816e15b3e7e090993dd
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:9bc5fa3b62b091f60614c08a7fb4fd1d3e1678e326f34dd66ce1eefb5dc3267b
parthea pushed a commit that referenced this issue Oct 21, 2023
)

Source-Link: https://togithub.com/googleapis/synthtool/commit/d6103f4a3540ba60f633a9e25c37ec5fe7e6286d
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:39f0f3f2be02ef036e297e376fe3b6256775576da8a6ccb1d5eeb80f4c8bf8fb
parthea pushed a commit that referenced this issue Oct 21, 2023
…424)

Source-Link: googleapis/synthtool@8e55b32
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:c6c965a4bf40c19011b11f87dbc801a66d3a23fbc6704102be064ef31c51f1c3

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this issue Oct 21, 2023
…[autoapprove] (#424)

Source-Link: googleapis/synthtool@4826337
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:60a63eddf86c87395b4bb394fdddfe30f84a7726ee8fe0b758ea132c2106ac75

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
parthea pushed a commit that referenced this issue Oct 22, 2023
…424)

Source-Link: googleapis/synthtool@8e55b32
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:c6c965a4bf40c19011b11f87dbc801a66d3a23fbc6704102be064ef31c51f1c3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. 🚨 This issue needs some love. triage me I really want to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants