Skip to content

Conversation

@seratch
Copy link
Contributor

@seratch seratch commented Nov 8, 2022

This pull request adds requestFileInfo option to MethodsClient#filesUploadV2 method for feature parity with Node and Python SDKs. See the following PRs for its context:

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

@seratch seratch added enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client labels Nov 8, 2022
@seratch seratch added this to the 1.27.0 milestone Nov 8, 2022
@seratch seratch self-assigned this Nov 8, 2022
@seratch seratch force-pushed the request-file-info-in-files-upload-v2 branch from f7e7899 to 6501edd Compare November 8, 2022 02:37
@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #1081 (480be25) into main (b1ea488) will increase coverage by 0.01%.
The diff coverage is 77.77%.

@@             Coverage Diff              @@
##               main    #1081      +/-   ##
============================================
+ Coverage     75.95%   75.97%   +0.01%     
- Complexity     3774     3776       +2     
============================================
  Files           413      413              
  Lines         11750    11754       +4     
  Branches       1152     1153       +1     
============================================
+ Hits           8925     8930       +5     
  Misses         2130     2130              
+ Partials        695      694       -1     
Impacted Files Coverage Δ
...om/slack/api/methods/impl/FilesUploadV2Helper.java 81.39% <77.77%> (+0.90%) ⬆️
...imits/metrics/impl/BaseMemoryMetricsDatastore.java 80.08% <0.00%> (+0.43%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@seratch seratch force-pushed the request-file-info-in-files-upload-v2 branch from 6501edd to 480be25 Compare November 8, 2022 03:55
Copy link
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

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

Added a nit, but this looks good to me 💯

underlyingException.getFileInfoResponses().add(fileInfo);
if (!fileInfo.isOk()) {
throw underlyingException;
if (v2Request.isRequestFileInfo()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I would tend to try and place this in its own function named something like fetchFile that returns the File object from fileInfo.getFile() or partialFileObject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. In this case, the part won't be reused elsewhere plus we have to pass v2Request
and underlyingException to the internal method, which is not so simple. So, let me go with the current code.

@seratch seratch merged commit c2d42a6 into slackapi:main Nov 8, 2022
@seratch seratch deleted the request-file-info-in-files-upload-v2 branch November 8, 2022 22:47
SotaNakajima pushed a commit to SotaNakajima/java-slack-sdk that referenced this pull request Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants