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 optional flag allowing wrapping responses in a parent class for J… #192

Merged
merged 5 commits into from
Oct 20, 2020

Conversation

rogebrd
Copy link
Contributor

@rogebrd rogebrd commented Oct 14, 2020

…S and TS client

  • Adds the --wrap-response-in flag which allows users to pass in a value
  • Response will be wrapped in the following way: wrapper<type> where wrapper is the string passed in with the flag, and type is the original response type

Checklist

General Contributing

  • Have you read the Code of Conduct and signed the CLA?

Is This a Code Change?

  • Non-code related change (markdown/git settings etc)
  • Code Change
  • Example/Test Code Change

Validation

  • Have you ran tox?
  • Do the tests pass?

…S and TS client

- Adds the `--wrap-response-in` flag which allows users to pass in a value
- Response will be wrapped in the following way: `wrapper<type>` where `wrapper` is the string passed in with the flag, and `type` is the original response type
@rogebrd rogebrd requested a review from connorworley October 14, 2020 02:37
@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #192 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   51.11%   51.17%   +0.06%     
==========================================
  Files          37       37              
  Lines        8340     8351      +11     
  Branches     1779     1781       +2     
==========================================
+ Hits         4263     4274      +11     
  Misses       3757     3757              
  Partials      320      320              
Flag Coverage Δ
#unit 51.17% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
stone/backends/js_client.py 77.77% <100.00%> (+1.65%) ⬆️
stone/backends/tsd_client.py 65.27% <100.00%> (+3.15%) ⬆️

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 75d4d3c...9d87b66. Read the comment docs.

rogebrd added a commit to dropbox/dropbox-sdk-js that referenced this pull request Oct 14, 2020
- Fix bug introduced in v6.0.0 with the addition of the DropboxResponse class
- Fixes issue ([#333](#333)) and ([#340](#340))
- Reflecting stone changes introduced in ([#192](dropbox/stone#192))
@rogebrd rogebrd requested review from aelawson and removed request for connorworley October 14, 2020 22:23
Copy link
Contributor

@aelawson aelawson left a comment

Choose a reason for hiding this comment

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

This seems fine to me

@@ -81,11 +87,21 @@ def _generate_route(self, route_schema, namespace, route):
if route.deprecated:
self.emit(' * @deprecated')

return_type = None
if self.args.wrap_response_in:
if route.result_data_type.__class__ != Void:
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there value in still doing this for Void type? e.g. if you are providing access to headers with a Response wrapper class like we do in the SDK?

fmt_type(route.result_data_type)))
return_type = None
if self.args.wrap_response_in:
if route.result_data_type.__class__ != Void:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@rogebrd rogebrd merged commit 87ece6e into master Oct 20, 2020
@rogebrd rogebrd deleted the add_ts_wrapper branch October 20, 2020 23:42
rogebrd added a commit to dropbox/dropbox-sdk-js that referenced this pull request Oct 21, 2020
- Fix bug introduced in v6.0.0 with the addition of the DropboxResponse class
- Fixes issue ([#333](#333)) and ([#340](#340))
- Reflecting stone changes introduced in ([#192](dropbox/stone#192))
rogebrd added a commit to dropbox/dropbox-sdk-js that referenced this pull request Oct 21, 2020
* Update return types from T to DropboxResponse<T>

- Fix bug introduced in v6.0.0 with the addition of the DropboxResponse class
- Fixes issue ([#333](#333)) and ([#340](#340))
- Reflecting stone changes introduced in ([#192](dropbox/stone#192))
* Regenerate routes with new stone version

Change Notes:

File Namespace:

- Add SearchOrderBy unioin
- Add SearchMatchTypeV2 union
- Update SearchOptions to include order_by
- Update SearchMatchV2 to include match_type

Team Audit Log Namespace:

- Add TeamBrandingPolicy union
- Add TeamBrandingPolicyChangedDetails struct
- Add TeamBrandingPolicyChangedType struct
- Update EventDetails to include team_branding_policy_changed_details
- Update EventType to include team_branding_policy_changed
- Update EventTypeArg to include team_branding_policy_changed

Team Reports Namespace:

- Deprecate reports/get_storage
- Deprecate reports/get_activity
- Deprecate reports/get_membership
- Deprecate reports/get_devices
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.

2 participants