immudb-py icon indicating copy to clipboard operation
immudb-py copied to clipboard

feat: add `recent_first` alternative sorting arg to `ImmudbClient.history()` method

Open NickAnderegg opened this issue 3 years ago • 1 comments
trafficstars

The current argument sortorder in ImmudbClient.history() is unclear from its name. Making it more confusing, it's the opposite of the typical Python convention for sort orders, where a boolean affecting sort order is typically False by default, e.g.:

sorted(iterable, /, *, key=None, reverse=False)

This change adds a new argument, recent_first, which has the same functionality as sortorder, but doesn't require reading the docs to understand what it means. I've implemented recent_first as a keyword-only argument and redefined sortorder as an optional positional-or-keyword argument.

This means that existing applications that call this method will be unaffected, but has the added advantage of still working as expected if someone gets confused about the argument names in the future. I've written it so that all of these calls would function identically:

client.history(key, offset, limit, True)
client.history(key, offset, limit, sortorder=True)
client.history(key, offset, limit, recent_first=True)
client.history(key, offset, limit, True, recent_first=True) # recent_first is ignored when sortorder is specified
client.history(key, offset, limit, sortorder=True, recent_first=True)
client.history(key, offset, limit, sortorder=True, recent_first=False)  # recent_first is ignored when sortorder is specified
client.history(key, offset, limit, sortorder=True, recent_first="xyz")  # recent_first is ignored when sortorder is specified

Because effectively specifying yes or no when the method asks what is our sort order? is very confusing!

NickAnderegg avatar Oct 14 '22 00:10 NickAnderegg

I would suggest to not add another parameter, just operate on the sortorder and make that statement in documentation

Adding new parameter, that actually only changes the other parameter is the same as renaming the first parameter, so maybe we should just rename sortorder

Razikus avatar Oct 20 '22 11:10 Razikus