| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@tseaver I just took out all the XXX discussion points from the commit and so removed the don't merge label. There were six places (in the old commit) where I was unsure of what was happening and wanted to discuss:
|
Sorry, something went wrong.
|
I don't have a problem with the implementation, but I'm not sure we have addressed what to do with the "don't make me think" usability issue introduced by removing "eager" loads. |
Sorry, something went wrong.
|
@tseaver Yeah for sure we've got to get a better handle on that. But if people typically get a bucket via storage.get_bucket(bucket_name) then properties will already be populated. Those crafting one by hand via Bucket(bucket_name) may fall into the class of more sophisticated users. |
Sorry, something went wrong.
|
@tseaver I restarted the Travis build and it kickstarted coveralls into action |
Sorry, something went wrong.
|
For the case where missing "eager" messages would hurt: should getters raise an exception (with a message indicating "call load()"?) |
Sorry, something went wrong.
|
What do you mean by eager messages? Also, should this discussion block this PR? (I see this PR as a tiny piece of moving towards whatever monolith change #632 represents.) Yes, I think we should raise in some situations. Or provide an load_properties=False boolean in the constructor. Or some other such thing. When we get the same level of implicit (and lazy-loading) support that we have in datastore, we could also make eager vs. declarative a setting that people could turn on. |
Sorry, something went wrong.
|
I meant to ask whether attempts to use not-yet-fetched properties should fail? E.g., Bucket.get_cors() will return an empty list if _load_properties has not yet been called, even though the server-side might actually have values; should it be an error? Essentially, this is the same as the "testing" problem you listed above (where you have to call bucket._reload_properties()): the PR deletes the "eager" tests (which called it implicitly), but leaves those paths in limbo. |
Sorry, something went wrong.
|
@tseaver Yes I think we should just add a dirty boolean (probably needs a better name) and a timestamp of "last retrieved" / "last synced". But for things like get_cors(), maybe we want them to make the HTTP request (i.e. not behave like a getter, but like an HTTP request). |
Sorry, something went wrong.
|
We need to decide then if we are going to keep any "non-dirty" state client side. Having to make multiple HTTP requests to fetch different bits of the state is not exactly performant, compared to the "lazy fetch" we have before this PR series. |
Sorry, something went wrong.
|
We just need a declarative get / load / reload so that users can populate the contents of _properties |
Sorry, something went wrong.
Also removing dead code: - _PropertyMixin.CUSTOM_PROPERTY_ACCESSORS (and subclasses use of this) - _PropertyMixin._get_property (only consumer of CUSTOM_PROPERTY_ACCESSORS)
|
@tseaver I just rebased (sorry I didn't realize #693 made this PR un-mergable). Shall we put these concerns in #632 as points to address? We certainly need a "lazy" / "auto" story, but it should be closer to the surface of user interaction and should happen at well-defined times (i.e. in constructor given optional argument). |
Sorry, something went wrong.
|
I guess we can move forward here. Trying to re-wire the whole API from the inside out is problematic: I'm pretty sure we should be defining the "desired" stories someplace, so that we know when we have arrived. |
Sorry, something went wrong.
|
@tseaver That's what #632 is for. High-level concepts we want to be true to. (It currently says nothing about an "eager" mode.) As far as "inside-out", at one point I thought about just starting a brand new module and just porting features over, but decided against it. |
Sorry, something went wrong.
Removing HTTP request in _PropertyMixin.properties.
|
I'd rather have "stories" which outlined what the developer experience actually looks like (vs. the "principles" bit in #632). Our current docs are missing this kind of "narrative" focus, which I feel to be a big deficiency: putting that off in another, tech-writer-maintained place (that doesn't exist yet) leaves the developer side unfocused. |
Sorry, something went wrong.
|
You can open an issue similar to #632 with our desired dev exp for storage in the meantime and then we can align with that. |
Sorry, something went wrong.
) Source-Link: googleapis/synthtool@0c7b033 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:08e34975760f002746b1d8c86fdc90660be45945ee6d9db914d1508acdf9a547 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
) Source-Link: googleapis/synthtool@0c7b033 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:08e34975760f002746b1d8c86fdc90660be45945ee6d9db914d1508acdf9a547 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Source-Link: googleapis/synthtool@d52e638 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:4f9b3b106ad0beafc2c8a415e3f62c1a0cc23cabea115dbe841b848f581cfe99 Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com> Co-authored-by: Victor Chudnovsky <vchudnov@google.com>
🤖 I have created a release *beep* *boop* --- ## [2.13.2](https://togithub.com/googleapis/python-bigtable/compare/v2.13.1...v2.13.2) (2022-10-20) ### Bug Fixes * Respect deadlines for column family operations ([#687](https://togithub.com/googleapis/python-bigtable/issues/687)) ([df2e64a](https://togithub.com/googleapis/python-bigtable/commit/df2e64a79bbd8b28d0991706607af99d539320d1)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
…rt flag (#688) * feat: add rest transport generation for clients * feat: add rest transport generation for clients * feat: add transport flag * refactor: moved template logic outside * fix: small fixes in transport option logic * test: added unit test for transport flag * test: add unit test for http option method * test: add unit test for http option method branch * fix: fix import paths * fix: style check issues * fix: more style check issues * fix: addressing pr reviews * fix: typo in test_method * fix: style check fixes
🤖 I have created a release \*beep\* \*boop\* --- ## [0.36.0](https://www.github.com/googleapis/gapic-generator-python/compare/v0.35.11...v0.36.0) (2020-11-14) ### Features * add rest transport generation for clients with optional transport flag ([#688](https://www.github.com/googleapis/gapic-generator-python/issues/688)) ([af59c2c](https://www.github.com/googleapis/gapic-generator-python/commit/af59c2c3c3d6b7e1f626c3fbc2c03f99ca31b4a4)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
…se (#688) * feat: add suport for mapping exceptions to rest callables * avoid wrapping errors for rest callable * fix lint issues * add test coverage * address PR comments * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * fix lint issues * fix for none type method --------- Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
* fix: Fix type hinting in collection.py Mostly adding `| None` on all fields that may have `None` as a default value. * fix: Use Union in type hinting Use Union to be compatible with 3.7 --------- Co-authored-by: Cedric Khin <ckhin@opensense.fr> Co-authored-by: Mariatta Wijaya <Mariatta@users.noreply.github.com>
Source-Link: googleapis/synthtool@53ea389 Post-Processor: gcr.io/repo-automation-bots/owlbot-python:latest@sha256:e1793a23ae0ee9aafb2e3a53b564a351f74790dbe3c2d75f8fc3b8c43e5c036c
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
| Back | FazBrowse Home | New Git URL |
NOTE 1: Has #683 as diffbase.
NOTE 2: There are many places here where the tests just give up and call _reload_properties(). This should not be. We should be clear on which methods should be just returning local data and which should make an HTTP request (e.g. update to Bucket.get_logging() here). (I also partially point out this type of problem in #683)
Also removing dead code: