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

Add request_file_info arg to files_upload_v2 method #1282

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

seratch
Copy link
Member

@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
@codecov
Copy link

codecov bot commented Oct 24, 2022

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

@eddyg
Copy link
Contributor

eddyg commented Oct 24, 2022

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
Member

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

Copy link
Member

@srajiang srajiang left a comment

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
Contributor

eddyg commented Oct 24, 2022

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

@seratch
Copy link
Member 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
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 Version: 3x web-client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants