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

Add limit= to get methods, make their signatures more uniform #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mk-fg
Copy link
Contributor

@mk-fg mk-fg commented Jan 12, 2022

  • Adding one missing request parameters, one key at a time...

  • Ordered keys to be more like rpc.proto, maybe e.g. get_range(self, range_start, range_end, *, ...) in all sigs can be used to avoid such ordering/additions ever breaking code in the future, but guessing it's not an issue atm anyway.

  • Added docstrings for couple parameters where they were missing, but dunno how these should look for sort_order and such enums, so skipped these here.

  • get_all() was missing type annotations.

@martyanov
Copy link
Owner

Thanks! I planned to wrap the rpc enums in custom enum types, like one from watch commands.

@martyanov
Copy link
Owner

Please see https://github.com/martyanov/aetcd/blob/master/aetcd/rtypes.py#L257 for example. I personally don't like the string literals here, pretty much easy to abuse.

@mk-fg
Copy link
Contributor Author

mk-fg commented Jan 12, 2022

Can probably be fixed in a follow-up commit or PR.

@martyanov martyanov added the enhancement New feature or request label Jan 13, 2022
@martyanov martyanov added this to the 1.0 milestone Jan 13, 2022
@mk-fg
Copy link
Contributor Author

mk-fg commented Jan 14, 2022

Added new enums for ordering in the second commit, a couple notes on those:

  • Made them comparable to case-insensitive strings (via shared _CaseInsensitiveStringEnum class).

    Idea is to allow people to type 'key' instead of 'KEY' or aetcd.rtypes.RangeSortTarget.KEY.
    Only downside seem to be having that small superclass and maybe visual inconsistency that it allows, but maybe allowing to avoid HARD_TO_READ_AND_TYPE UPPERCASE is worth it.
    Feel free to just drop it though - should be as easy as removing that superclass and adjusting tests.

  • sort_order/target no longer accept None, only proper enum values.

    Seems a bit weird to have one special value there, which is then always translated to enum value under the hood, i.e. not actually optional in any way.
    In sort_target it's also translated into 'key', which seems a bit counter-intuitive, so maybe better to just ask for proper enum match in all cases?

    This is incompatibility, but same as mentioned in other PRs, maybe ok to introduce at this point.
    Should be easy to revert by using typing.Optional for types and something like rtypes.RangeSortOrder(sort_order or 'none').name in _build_get_range_request.

  • Haven't build docs to test how new descriptions look there, but they are very similar to existing ones, so barring any typos, should probably be fine.

@martyanov
Copy link
Owner

martyanov commented Jan 14, 2022

Why not just SortOrder and SortTarget as names for the enums?

Not sure if I like the _CaseInsensitiveStringEnum, I prefer to be more strict and explicit. :) Breaking the API at the moment is not an issue at all, alpha status of the prject is reflected in its version and project meta.

I'll take a closer look. Thanks!

@martyanov
Copy link
Owner

A little follow up. If you make the enums importable via aetcd, as all other rtypes types, then it's just a aetcd.SortTarget.KEY. Personally I'm fine with this.

@mk-fg
Copy link
Contributor Author

mk-fg commented Jan 14, 2022

Why not just SortOrder and SortTarget as names for the enums?

They are only relevant for RangeRequest and translated to RangeRequest.SortOrder, so seemed fitting to namespace them like that, similar to how EventKind is not just Kind, but obviously doesn't really matter.

@mk-fg
Copy link
Contributor Author

mk-fg commented Jan 14, 2022

Yeah, I think with strings I was also a bit influenced by how EventKind is halfway str + enum type.
So seemed to be best to either make them all abstract enums not in any way comparable to strings (as that leads to typos which python can't check), or - if they are string - make them human-friendly strings.

Guess if enums are fine, should get rid of all "str" in there and only ever use/allow enums.

@martyanov
Copy link
Owner

martyanov commented Jan 14, 2022

Yep, but only the enums should be explicitly imported, as other types return implicitly as operation results, so they only needed may be for type annotations. I think the short enum names are self descriptive enough, readability counts. And we dont have many of them in proto definitions.

@martyanov
Copy link
Owner

Enum-only variant sounds good to me.

@mk-fg mk-fg force-pushed the add-limit-to-get-requests branch 2 times, most recently from 87c1a12 to aa7e17a Compare January 14, 2022 18:04
@mk-fg
Copy link
Contributor Author

mk-fg commented Jan 14, 2022

EventKind, SortOrder, SortTarget should be all auto-value enums now, with None or str passed to sort_order/target options raising ValueError.

Repository owner deleted a comment from H4wk-eye Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants