-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix files_list #18
Fix files_list #18
Conversation
The problem I see there is how to know what fields Slack thinks are always there. In #17 you fixed that What do you think, does that sound sensible? |
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 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. :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is a discussion for another time, so I would recommend just removing the comments and then I'd merge it as-is.
Due to the amount of option
types, we could consider bundling own option
map and bind operators for people to access these values in a more convenient way.
src/slacko.ml
Outdated
url_download: string; | ||
(* These two are deprecated and appear to be gone. *) | ||
(* url: string; *) | ||
(* url_download: string; *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they are gone, then we can safely remove them, no need to retain them as comments, a note in the commit message should suffice.
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.