Skip to content

Add request_file_info arg to files_upload_v2 method#1282

Merged
seratch merged 2 commits intoslackapi:mainfrom
seratch:fils-upload-v2-file-data
Oct 25, 2022
Merged

Add request_file_info arg to files_upload_v2 method#1282
seratch merged 2 commits intoslackapi:mainfrom
seratch:fils-upload-v2-file-data

Conversation

@seratch
Copy link
Copy Markdown
Contributor

@seratch seratch commented Oct 24, 2022

Summary

This pull request adds a new optional flag argument to files_upload_v2 method. When a developer passes full_file_info_required=False, the method skips fetching files.info response data for the uploaded files. Refer to #1277 for the context behind this change.

@srajiang I am thinking of adding the same flag to your Node SDK implementation. If you have any suggestions on better naming, I am happy to align for it.

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

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs-src (Documents, have you run ./scripts/docs.sh?)
  • /docs-src-v2 (Documents, have you run ./scripts/docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

@seratch seratch added enhancement M-T: A feature request for new functionality web-client Version: 3x labels Oct 24, 2022
@seratch seratch added this to the 3.19.2 milestone Oct 24, 2022
@seratch seratch requested a review from srajiang October 24, 2022 02:54
@seratch seratch self-assigned this Oct 24, 2022
@seratch seratch mentioned this pull request Oct 24, 2022
Closed
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 24, 2022 •
edited
Loading

Codecov Report

Merging #1282 (cbc106f) into main (f47f5cc) will increase coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1282      +/-   ##
==========================================
+ Coverage   85.63%   85.67%   +0.04%     
==========================================
  Files          84       84              
  Lines        7826     7827       +1     
==========================================
+ Hits         6702     6706       +4     
+ Misses       1124     1121       -3
Impacted Files Coverage Δ
slack_sdk/web/legacy_client.py 83.25% <0.00%> (-0.08%) ⬇️
slack_sdk/socket_mode/builtin/internals.py 73.81% <0.00%> (+1.71%) ⬆️

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

@seratch seratch force-pushed the fils-upload-v2-file-data branch from dbc14c0 to d29fe50 Compare October 24, 2022 03:35
@eddyg
Copy link
Copy Markdown
Contributor

eddyg commented Oct 24, 2022 •
edited
Loading

This look great, thanks for taking the time to add this option! 🎉

My only concern is with the name full_file_info_required. Since the call to files.info isn't guaranteed to always contain a full set of data, I'm worried some users may think the term required implies there's a "contract" that ensures the full set of metadata does get returned.

I've been trying to think of a better name for the option. Perhaps request_full_file_info?

Thanks again!

@srajiang
Copy link
Copy Markdown
Contributor

srajiang commented Oct 24, 2022

Fair concerns @eddyg's around the required part of the naming! How about we also remove full for a similar reason to what you mentioned re: not guaranteeing a "full" data around a file, and go with simply request_file_info?

@seratch I'll get this added to node implementation

srajiang
srajiang approved these changes Oct 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@srajiang srajiang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM once we align on a name!

@eddyg
Copy link
Copy Markdown
Contributor

eddyg commented Oct 24, 2022

Sarah, FWIW, I like your name suggestion for the reason you gave. 👍

@seratch
Copy link
Copy Markdown
Contributor Author

seratch commented Oct 25, 2022

Thanks, request_file_info looks good to me too

@seratch seratch merged commit 5f800c1 into slackapi:main Oct 25, 2022
@seratch seratch deleted the fils-upload-v2-file-data branch October 25, 2022 03:05
@seratch seratch changed the title Add full_file_info_required to files_upload_v2 method Add request_file_info arg to files_upload_v2 method Oct 25, 2022
@seratch seratch mentioned this pull request Oct 26, 2022
Closed
@seratch seratch mentioned this pull request Nov 8, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@srajiang srajiang srajiang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@seratch seratch

Labels

enhancement M-T: A feature request for new functionality Version: 3x web-client

Projects

None yet

Milestone

3.19.2

Development

Successfully merging this pull request may close these issues.

3 participants

@seratch @eddyg @srajiang