Added support for Smartsheet-Integration-Source#75
Added support for Smartsheet-Integration-Source#75ahmed-ahmed-smartsheet wants to merge 5 commits intosmartsheet:mainlinefrom
Conversation
Rurquhart
left a comment
Rurquhart
left a comment
There was a problem hiding this comment.
Could we please add some test cases to cover the functionality? E.g Testing that a header is included in requests once specified
|
|
||
| ### Added | ||
|
|
||
| - Support for the `Smartsheet-Integration-Source` header. This can be configured using the `smartsheet.py`. |
There was a problem hiding this comment.
This can be configured using the Smartsheet class.
| access_token (str): Access Token for making client | ||
| requests. May also be set as an env variable in | ||
| SMARTSHEET_ACCESS_TOKEN. (required) | ||
| smartsheet_integration_source (str): Integration source identifier. |
There was a problem hiding this comment.
Wouldn't it make sense to make this a class with type, org_name and integrator_name instead of string that the client needs to fiddle with?
| access_token (str): Access Token for making client | ||
| requests. May also be set as an env variable in | ||
| SMARTSHEET_ACCESS_TOKEN. (required) | ||
| smartsheet_integration_source (str): Integration source identifier. |
There was a problem hiding this comment.
I think this could just be named integration_source. The smartsheet prefix is not necessary having in mind that the class name is Smartsheet.
| pass | ||
|
|
||
| if self._smartsheet_integration_source is not None: | ||
| prepped_request.headers.update( |
There was a problem hiding this comment.
The dict.update() method is overkill here to set a single key. You should just use prepped_request.headers["Smartsheet-Integration-Source"] = self._smartsheet_integration_source
| ) | ||
| else: | ||
| try: | ||
| del prepped_request.headers["Smartsheet-Integration-Source"] |
There was a problem hiding this comment.
Why the deletion in the else case?
Also it's better to use prepped_request.headers.pop("Smartsheet-Integration-Source") without try/catch instead of del.
| Validates a smartsheet integration source string in the format: | ||
| type, organisation name, integrator name | ||
|
|
||
| - type: must be one of the enum values |
There was a problem hiding this comment.
must be one of the SmartsheetIntegrationSourceType enum values.
| - integrator name: non-empty | ||
| """ | ||
| if input_value is None: | ||
| raise SmartsheetException("Smartsheet integration source cannot be null") |
There was a problem hiding this comment.
The error message should use None instead of null: Smartsheet integration source cannot be None
| if len(parts) != 3: | ||
| raise SmartsheetException( | ||
| "Invalid smartsheet integration source format. " | ||
| f"Expected format: 'TYPE,ORGANIZATION,INTEGRATOR. {DOCUMENTATION_LINK}" |
There was a problem hiding this comment.
The message here has an opening single quote but the closing one is missing.
| ) | ||
|
|
||
| # Validate Smartsheet integration source format | ||
| is_valid_format(smartsheet_integration_source) |
There was a problem hiding this comment.
Here you can use with_smartsheet_integration_source to avoid duplication.
| f"Expected format: 'TYPE,ORGANIZATION,INTEGRATOR. {DOCUMENTATION_LINK}" | ||
| ) | ||
|
|
||
| integration_type = parts[0] |
There was a problem hiding this comment.
These are trimmed in the C# SDK. Maybe its worth doing it here as well for consistency.
3 participants
Pull Request
*** DO NOT MERGE ***
Description
Type of Change
Environment Information
What Changes Were Made
Added Smartsheet-Integration-Source request header support
Why These Changes Were Made
The header is needed for integration with the new Public APIs
Testing
Added mock for the new header in test_mock_change_agent.py
Checklist