Leonidas-from-XIV / slacko

A neat interface for Slack
https://leonidas-from-xiv.github.io/slacko
Other
81 stars 11 forks source link

Fix files_list #18

Closed jerith closed 7 years ago

jerith commented 7 years ago

This addresses one of the items in #10.

I've tested this using my code from #14 in a mostly unused slack team that only has default files. I think I've made all the necessary fields optional, but it's hard to tell for sure.

Idea for later: Maybe for some object types we should limit the records to the core fields that everything has and then also return all the other fields along with some tools for getting their values? That way the caller gets the information they probably care about in a nice format, but can still get at the obscure (and sometimes undocumented) fields that only some instances have without needing to update slacko every time slack adds a new one.

Leonidas-from-XIV commented 7 years ago

The problem I see there is how to know what fields Slack thinks are always there. In #17 you fixed that tz is optional, but I believe it used to be always there with a default value. The underlying problem is that the API documentation is a mess. I would wish for a formal description of the API that we could just parse. The other downside is when you ask for the fields as a string, you throw away all the type information, so I would prefer to keep the option keys for all the fields we know about. What we could have, though is a function to attempt to retrieve a key we don't yet know about: e.g. a get_key name function attached to the resulting record which is partially applied with the yojson parse result and can look up a key in the result.

What do you think, does that sound sensible?

jerith commented 7 years ago

Yes, that sounds sensible.

Another option is to provide lookup functions that parse groups of related fields (the mess of thumbnails, for example) into records on demand for things that aren't likely to be used by the majority of callers. That would let us keep the core field set fairly conservative (and thus more reliable across API changes) and reduce the number of options in the records by providing defaults instead (where possible). The full JSON would still be available for (through the mechanism you proposed) for anything that isn't explicitly supported.

Either way, I think this conversation deserves its own ticket. I'd like to get the test stuff and a few smaller fixes finished before tackling nontrivial refactoring efforts. :-)