search

osuosl / timesync

A time tracking API
2 stars 1 forks source link

Boolean GET parameters need more explanation #78

Closed mathuin closed 7 years ago

mathuin commented 7 years ago

https://timesync.readthedocs.io/en/latest/api.html#get-request-query-parameters

It is unclear from the documentation whether the include_revisions or include_deleted query parameters default to true or false. If they default to false, then I'm wondering why they're true/false at all -- wouldn't the presence of the flag be sufficiently indicative of the query's intent?

MorganEPatch commented 7 years ago

They do in fact default false, and the presence of the parameter is sufficient to indicate that they should be true. When talking about HTTP query parameters, a "boolean" parameter is one whose value is determined by its presence, precisely as you indicate. GET /times?token=... has both set to false, and GET /times?include_deleted=&token=... has include_deleted set to true.

mathuin commented 7 years ago

Are you positive that GET /times?include_deleted=&token=... forces include_deleted to true? That's not supported by the documentation.

As it stands, according to the docs the results returned by GET /times?token=... are expected to be no different than GET /times?include_deleted=false&token=... which begs the question of why these parameters have settings at all.

A somewhat-quick tour through RFC 2616 shows that query parameters don't need values to be valid so maybe there is no reason for these parameters to have any values other than their presence.

Before we go /v1, someone should consider making these parameters work without true or false, just by their presence. GET /times?include_deleted&token=... should be adequate.

The other issue, of cleaning up the parameters before /v1, I can make as a separate issue if you'd prefer.

MorganEPatch commented 7 years ago

GET /times?token=... returns only active times

GET /times?include_deleted&token=... returns all times

GET /times?include_deleted=&token=... returns all times

GET /times?include_deleted=true&token=... returns all times

GET /times?include_deleted=foo&token=... returns all times

GET /times?include_deleted=false&token=... returns only active times.

Perhaps the documentation isn't very clear, but the idea is that passing any value for that parameter except false (including nothing) will set it to true, but passing specifically the value false will set it to false. I think it's generally a good idea to have an explicit opt-out value, even if it defaults to off anyway.