Skip to content
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

Generalize handling of arguments with binary value types #519

Merged
merged 4 commits into from
Mar 23, 2018

Conversation

aoberoi
Copy link
Contributor

@aoberoi aoberoi commented Mar 22, 2018

Summary

based on #513 - as this PR states, this should allow methods like users.setPhoto to also work properly.

fixes #452

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Mar 22, 2018

Codecov Report

Merging #519 into master will decrease coverage by 0.28%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
- Coverage    79.7%   79.42%   -0.29%     
==========================================
  Files           6        6              
  Lines         271      277       +6     
  Branches       42       42              
==========================================
+ Hits          216      220       +4     
- Misses         38       39       +1     
- Partials       17       18       +1
Impacted Files Coverage Δ
src/WebClient.ts 81.25% <88.88%> (-0.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b957324...7a97825. Read the comment docs.

@aoberoi aoberoi mentioned this pull request Mar 22, 2018
2 tasks
});
});

// Reactivate this test once we find out if the workaround in the test case before is necessary
it('should log a warning when file is a Buffer and there is no filename', function () {
// TODO: re-enable this once we decide if we want to generate a random name or if we want to log a warning
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an open point we should resolve before merging this.

@aoberoi
Copy link
Contributor Author

aoberoi commented Mar 22, 2018

blocker: i just tried using this implementation with a Buffer (with or without a filename argument) and it failed with the following error:

   { ok: false,
     error: 'no_file_data',
     acceptedScopes: [ 'files:write:user', 'post' ] } }

@aoberoi
Copy link
Contributor Author

aoberoi commented Mar 22, 2018

okay, i've learned an interesting thing. the following is true for files.upload (i haven't checked against other methods, but i will):

if in the multipart body, the part that contains the binary data does not specify a filename attribute in the header, the upload will fail. if another part contains a filename that will override whatever was specified in the filename attribute of the first part.

@aoberoi
Copy link
Contributor Author

aoberoi commented Mar 22, 2018

and the same is true for users.setPhoto. that is that if no filename attribute is specified, it will fail. in this case, there's no need for an additional filename part, since the filename is generated by Slack.

@aoberoi
Copy link
Contributor Author

aoberoi commented Mar 23, 2018

side note: love it when you implement a new feature with more deletions than additions 💯

@@ -656,6 +651,8 @@ export interface WebAPIHTTPError extends CodedError {
* Helpers
*/

const defaultFilename = 'Untitled';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a note to the docs for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh, if we had generated docs for the individual methods at all, then i'd say yes.

@aoberoi aoberoi merged commit e9a5e2d into slackapi:master Mar 23, 2018
@aoberoi aoberoi deleted the clavin-feat-webclient-binary-data branch March 23, 2018 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reactions.get Facet fails to send request when requesting "full: true" and "file: id"
3 participants