Page MenuHomeSoftware Heritage

Make origin_visit_get_random return None instead of {} if there are no results.
ClosedPublic

Authored by vlorentz on Jan 16 2020, 4:18 PM.

Diff Detail

Repository
rDSTO Storage manager
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Jan 16 2020, 4:18 PM
ardumont accepted this revision.Jan 16 2020, 5:55 PM
ardumont added a subscriber: ardumont.

In that case, I prefer an empty dict or an empty list.
Instead of Optional[Dict] or Optional[List], which i found convoluted (especially now that we are adding types)...

Also, like i said on irc, not all endpoints behave as mentioned, there is content_get_metadata which returns a Dict[bytes, List[Dict]].

Anyway, my main concern is that at some point, we should decide whether to continue having those endpoints returns None.
the arguments "all other endpoints are like that" is getting old on me.
Consistency yes, but good consistency.
And I'm not sure we are there.

Note:
it's not limited to that, I also would like we align inputs, stop using generator for some, then list for others, ...

This revision is now accepted and ready to land.Jan 16 2020, 5:55 PM
vlorentz added a comment.EditedJan 17 2020, 2:58 PM

In that case, I prefer an empty dict or an empty list.
Instead of Optional[Dict] or Optional[List], which i found convoluted (especially now that we are adding types)...

using Optional in the signature makes it explicit that we may return no object.

Also, like i said on irc, not all endpoints behave as mentioned, there is content_get_metadata which returns a Dict[bytes, List[Dict]].

That's different. content_get_metadata returns multiple objects, and there may be 0, 1, or many results. So possibly-empty dicts and list are ok there.

But origin_visit_get_random returns either 0 or 1 result, so it's None for 0 and a non-empty dict for 1.

Maybe it's my FP/ADT background leaking, in that for me, "X = always 1 object", "Option<X> = 0 or 1 object", "container of X = 0 or 1 or many objects". (I'm also disappointed there's generally no way to express "1 or many objects" at the type level; some people tried that in Rust but it's not convincing, eg. https://www.reddit.com/r/rust/comments/8rst01/any_peeps_interested_in_developping_a_fast/ )

Note:
it's not limited to that, I also would like we align inputs, stop using generator for some, then list for others, ...

I'm all for it. If you look at the code of the Cassandra backend, there are a bunch of TODOs about changing names of arguments, but I can't do it without breaking the RPC API (which passes argument names)

ardumont added a comment.EditedJan 17 2020, 3:04 PM

Maybe it's my FP or ADT background leaking, in that for me, "X = always 1 object", "Option<X> = 0 or 1 object", "container of X = 0 or 1 or many objects".

great, i see it that way as well.

it's just that as usual not {} or not [] which tends to blur the identity matter...

Also if we stopped using dict but object, that'd be clearer... Optional[OriginVisit] is the type we want here, i think.
I think that's in the pipe ;)

Also if we stopped using dict but object, that'd be clearer... Optional[OriginVisit] is the type we want here, i think.
I think that's in the pipe ;)

Oh, yes. It will be wonderful to no longer have dicts around anymore, but we're stuck with them for now. Maybe we can start using Model objects at the same time we move to a new RPC framework (@douardda mentioned gRPC)