[rest] change text from a property to a method #20290

iscai-msft · 2021-08-16T17:36:41Z

No description provided.

sdk/core/azure-core/azure/core/rest/_requests_basic.py

iscai-msft · 2021-08-19T15:59:04Z

sdk/core/azure-core/tests/testserver_tests/test_rest_http_response.py

+    assert response.text() == u"👩"
+
+    # try setting encoding as a property
+    response.encoding = "utf-16"


this decodes it with utf-16

iscai-msft · 2021-08-19T15:59:16Z

sdk/core/azure-core/tests/testserver_tests/test_rest_http_response.py

+        request=HttpRequest("GET", "/encoding/emoji"),
+    )
+    assert response.content == u"👩".encode("utf-8")
+    assert response.text() == u"👩"


we start off with the default utf-8-sig

iscai-msft · 2021-08-19T15:59:28Z

sdk/core/azure-core/tests/testserver_tests/test_rest_http_response.py

+    assert response.text() == u"鿰ꦑ" == response.content.decode(response.encoding)
+
+    # assert latin-1 changes text decoding without changing encoding property
+    assert response.text("latin-1") == 'ð\x9f\x91©' == response.content.decode("latin-1")


this decodes it with latin-1

sdk/core/azure-core/azure/core/rest/_rest.py

xiangyan99

LGTM

iscai-msft · 2021-08-19T17:08:13Z

/azp run python - autorest - pr

azure-pipelines · 2021-08-19T17:08:23Z

Azure Pipelines successfully started running 1 pipeline(s).

johanste · 2021-08-19T17:13:34Z

sdk/core/azure-core/azure/core/rest/_rest_py3.py

            content = self.content
            if not content:
                self._text = ""
            else:
-                self._text = decode_to_text(self.encoding, self.content)
+                encoding_to_pass = encoding or self.encoding
+                self._text = decode_to_text(encoding_to_pass, self.content)


Why use self.content when you fished out the value on line 275. Alternatively, why are you fishing out content on line 275? Do we want to save the instructions needed to access the attribute from the instance?

Is it because accessing the content property has side-effects?

Acccessign the content property will raise the ResponseNotRead errors if users mishandle. You ahve a good point, I don't have to store it in another variable, so I'll just stick to using self.content

I agree with pylint that the side effects of accessing self.content are too subtle. You are going to access self.content anyways, why do you need it also call it here?

sorry i'm not quite understanding your comment. Does the current code there look better to you?

Why not just say:

if not self.content: # <- this will raise if data is not read. Line 275 is not needed. self._text = "" else: ...

ah i think i was confused bc we're looking at diff code versions. What I currently have is

if self._text is None or encoding: encoding_to_pass = encoding or self.encoding self._text = decode_to_text(encoding_to_pass, self.content) return self._text

which i think solves the issue you brought up (i moved the empty content detection to decode_to_text

iscai-msft · 2021-08-19T19:07:25Z

/azp run python - core - ci

azure-pipelines · 2021-08-19T19:07:44Z

Azure Pipelines successfully started running 1 pipeline(s).

…into use_azure_core_endcoder * 'main' of https://github.com/Azure/azure-sdk-for-python: [Key Vault] Add 7.3-preview support for administration (#20364) Fix Monitor opentelemetry exporter readme issues (#19038) More Renaming in query (#20303) Update CODEOWNERS (#20366) [ServiceBus] update migration guide with message count info (#20360) [rest] change text from a property to a method (#20290) Handle value types for results (#20358)

…into header_tuples * 'main' of https://github.com/Azure/azure-sdk-for-python: (104 commits) [Key Vault] Add 7.3-preview support for administration (#20364) Fix Monitor opentelemetry exporter readme issues (#19038) More Renaming in query (#20303) Update CODEOWNERS (#20366) [ServiceBus] update migration guide with message count info (#20360) [rest] change text from a property to a method (#20290) Handle value types for results (#20358) Remove old unused update changelog script (#20357) bump node version (#20353) [AutoRelease] t2-web-2021-08-03-73015 (#20053) Fix query batch processing (#20345) Sync eng/common directory with azure-sdk-tools for PR 1912 (#20340) Increment version for schemaregistry releases (#20326) [SchemaRegistry] prepare avro for release (#20321) Update CHANGELOG.md (#20334) Update main.py (#20332) Removing C:\Git\azure-sdk-tools\eng\common\scripts\FilterPoliCheckResults.ps1 as it was no longer used (#20325) Suppress CredScan warning for test proxy devcert (#20324) [SchemaRegistry] update samples readme (#20323) [Test Proxy] Add RecordedByProxy decorator and AzureRecordedTestCase (#20138) ...

…into have_pipelines_support_rest * 'main' of https://github.com/Azure/azure-sdk-for-python: (27 commits) Document Pod Identity's special use of client_id (Azure#20377) [rest] use azure json encoder for json input bodies (Azure#20361) CertificateCredential supports PKCS12 certs (Azure#16384) Bug fix for pipeline downloading incorrect package ver (Azure#20294) Update metadata values (Azure#20365) [Key Vault] Add 7.3-preview support for administration (Azure#20364) Fix Monitor opentelemetry exporter readme issues (Azure#19038) More Renaming in query (Azure#20303) Update CODEOWNERS (Azure#20366) [ServiceBus] update migration guide with message count info (Azure#20360) [rest] change text from a property to a method (Azure#20290) Handle value types for results (Azure#20358) Remove old unused update changelog script (Azure#20357) bump node version (Azure#20353) [AutoRelease] t2-web-2021-08-03-73015 (Azure#20053) Fix query batch processing (Azure#20345) Sync eng/common directory with azure-sdk-tools for PR 1912 (Azure#20340) Increment version for schemaregistry releases (Azure#20326) [SchemaRegistry] prepare avro for release (Azure#20321) Update CHANGELOG.md (Azure#20334) ...

* CodeGen from PR 15691 in Azure/azure-rest-api-specs [NetAppFiles] fix default in volumeproperties (#15691) * fix default in volumeproperties * Update description * Update description Co-authored-by: Audunn Baldvinsson <audunn.baldvinsson@netapp.com> * version,CHANGELOG * test * bump node version (#20353) * Remove old unused update changelog script (#20357) Co-authored-by: Wes Haggard <Wes.Haggard@microsoft.com> * Handle value types for results (#20358) * Handle value types for results * update test * lint * comprehension * more precis * fix test * [rest] change text from a property to a method (#20290) * [ServiceBus] update migration guide with message count info (#20360) #20245 * Update CODEOWNERS (#20366) * More Renaming in query (#20303) * More Reanaming in query * changelog * commit 2 * some changes * remove errror * Update sdk/monitor/azure-monitor-query/azure/monitor/query/_models.py * Apply suggestions from code review Co-authored-by: Adam Ling (MSFT) <adam_ling@outlook.com> * Update sdk/monitor/azure-monitor-query/CHANGELOG.md Co-authored-by: Adam Ling (MSFT) <adam_ling@outlook.com> * Fix Monitor opentelemetry exporter readme issues (#19038) * [Key Vault] Add 7.3-preview support for administration (#20364) * Update metadata values (#20365) * Update metadata values * distributed trace * oops * Update sdk/monitor/azure-monitor-query/CHANGELOG.md * Apply suggestions from code review * Bug fix for pipeline downloading incorrect package ver (#20294) * Bug fix: pipeline to download correct package ver * Update clean version * Update clean version * Cleanup * Cleanup * Cleanup * Cleanup * Comments added * Testing * Final-Version * CertificateCredential supports PKCS12 certs (#16384) * [rest] use azure json encoder for json input bodies (#20361) * Document Pod Identity's special use of client_id (#20377) * Consistency related changes (#20385) * time stamp * Rename AggragationType to MetricAggregationType * Metric Class * logs batch result * MetricNamespaceClassification * LogsTable + LogsTableColumn * lint * more lint * Add Cloud Configuration section to Identity readme (#20373) * keyvault and storage have a conflict between mindependency and what local azure-identity requires. resolve it (#20391) * [AutoRelease] t2-costmanagement-2021-07-27-84467 (#19949) * CodeGen from PR 15291 in Azure/azure-rest-api-specs Updating Query filter description (#15291) Co-authored-by: Jorge Chavez Nieto <jochav@microsoft.com> * version,CHANGELOG Co-authored-by: SDKAuto <sdkautomation@microsoft.com> Co-authored-by: Jorge Chavez Nieto <jochav@microsoft.com> Co-authored-by: PythonSdkPipelines <PythonSdkPipelines> * [AutoRelease] t2-servicebus-2021-08-19-00756 (#20350) * CodeGen from PR 14749 in Azure/azure-rest-api-specs add apitestErrorCode doc (#14749) * add apitestErrorCode doc * add armRPC doc * add roundTripInconsistentProperty doc * small fix * update * update signalR armTemplate * update yaml * new file * add testScenario with armTemplate * update doc * update readme.md * update doc * update NOTE section * add signalRCreateOrUpdate example file * udpate doc * add generate test scenario section * update doc * add serviceFacbric test scenario file * update managedClusters.yaml * update generateABasicTestScenario.md * update features doc * add run api test gif * version,CHANGELOG * test * Update test_cli_mgmt_servicebus_namespace.py * Update test_queues.py * Update test_queues.py Co-authored-by: SDKAuto <sdkautomation@microsoft.com> Co-authored-by: PythonSdkPipelines <PythonSdkPipelines> Co-authored-by: Zed Lei <59104634+RAY-316@users.noreply.github.com> Co-authored-by: Jiefeng Chen <51037443+BigCat20196@users.noreply.github.com> * [AutoRelease] t2-policyinsights-2021-08-23-78735 (#20381) * CodeGen from PR 15165 in Azure/azure-rest-api-specs [policyinsights] Fix next_link (#15165) * fix next_link * directive * Update readme.nodejs.md * js directvie * Update readme.md * supress other language * Update readme.go.md * swagger lint * swagger lint * swagger lint * Update readme.md * version,CHANGELOG Co-authored-by: SDKAuto <sdkautomation@microsoft.com> Co-authored-by: PythonSdkPipelines <PythonSdkPipelines> Co-authored-by: Zed Lei <59104634+RAY-316@users.noreply.github.com> * [AutoRelease] t2-datamigration-2021-08-19-04035 (#20349) * CodeGen from PR 15259 in Azure/azure-rest-api-specs Dev datamigration microsoft.data migration 2021 06 30 (#15259) * Created new api version and refactoring changes (such as fixing new build restrictions) * Adding breaking changes * Adding location to the services update example * Adding location for projects update and refactoring the task command * Resolving tasks command ModelValidation test failure * Resolving issue based on zhenglaizhang's recommendation * Adding comma to resolve issue Co-authored-by: Artem Pavlichenko <arpavlic@microsoft.com> * version,CHANGELOG Co-authored-by: SDKAuto <sdkautomation@microsoft.com> Co-authored-by: Artem Pavlichenko <arpavlic@microsoft.com> Co-authored-by: PythonSdkPipelines <PythonSdkPipelines> * Chain exceptions from LibsecretPersistence (#20380) * [translation] fix poller.details (#20392) * fixing docstring types for async client * populate poller.details.id immediately * pylint * add bug fix to changelog * improve begin_translation docstring (#20401) * Update .docsettings.yml (#20402) * Sync eng/common directory with azure-sdk-tools for PR 1918 (#20342) * Stress test deployment script resiliency fixes * Use PSModule-Helpers to install powershell-yaml Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> * improve docs and samples for glossaries and custom models (#18587) * update the readme * update readme file * added custom translation samples * fix 'no-locale' thing in links * update glossary docs * update glossaries * link to sample glossaries instead of writing code in readme * update custom model sample linking * remove relative linking in readme * make subheadings in bold text to be more readable * conform with 'Document Translation' naming * disambiguate container sas url * capitaliz Azure name * remove misplaced period * update samples -> custom model * update async sample -> custom model * remove localization from url * update readme with new file types for glossaries * adding sample glossaries -> xlf * white space * use simplified single input method * update 'job' terminology * update azure-core naming * update glossary blob file reference name * link to supported glossaries table * remove locale from url * [purview account] first release for azure-purview-account (#20335) * purview account * CI * verion-tolerant * endpoint order * add test for collections * Update CHANGELOG.md * review * update reamdme * skip custom translation model samples (#20414) * Address API view issues (#20407) * logs client * lc-2 * models changes * more changes * more changes * more arch changes * changelog * tests * lint * fix tests * timespan * [translation] naming alignment (#20406) * list_all_translation_statuses -> list_translation_statuses * list_all_document_statuses -> list_document_statuses * cancelled -> canceled, cancelling -> canceling * FileFormat -> DocumentTranslationFileFormat * update changelog * make core tests offline (#19986) * make core tests offline * clean up * use localhost instead of 127.0.0.1 * update * update * update * Get rid of LogsBatchQueryResult (#20418) * Get rid of LogsBatchQueryResult * Update sdk/monitor/azure-monitor-query/README.md * oops * comments * [AutoRelease] t2-iothub-2021-08-25-25696 (#20409) * CodeGen from PR 15722 in Azure/azure-rest-api-specs Removing readonly for isVerified property as it is no more readonly property (#15722) * version,CHANGELOG * test Co-authored-by: SDKAuto <sdkautomation@microsoft.com> Co-authored-by: PythonSdkPipelines <PythonSdkPipelines> * [AutoRelease] t2-healthcareapis-2021-08-26-27542 (#20422) * CodeGen from PR 15381 in Azure/azure-rest-api-specs Update readme.md (#15381) * version,CHANGELOG Co-authored-by: SDKAuto <sdkautomation@microsoft.com> Co-authored-by: PythonSdkPipelines <PythonSdkPipelines> * Stip empty changelog sections before release (#20437) Co-authored-by: Chidozie Ononiwu <chononiw@microsoft.com> * Update question-answering readme links (#20439) * ensure test principal creation succeeds properly (#20446) Co-authored-by: scbedd <45376673+scbedd@users.noreply.github.com> * New dummy password in README (#20328) * Sync eng/common directory with azure-sdk-tools for PR 1943 (#20450) * Bug fix: Storing .env file for bicep file at service dir * Styling Co-authored-by: Albert Cheng <albertcheng@microsoft.com> * [AutoRelease] t2-keyvault-2021-08-26-55443 (#20423) * CodeGen from PR 15458 in Azure/azure-rest-api-specs Add rotation permission (#15458) * Add rotation permission * Update keys.json Remove required from lifetimeActions * Change to expiryTime * version,CHANGELOG * test Co-authored-by: SDKAuto <sdkautomation@microsoft.com> Co-authored-by: PythonSdkPipelines <PythonSdkPipelines> * new recordings Co-authored-by: SDKAuto <sdkautomation@microsoft.com> Co-authored-by: Audunn Baldvinsson <audunn.baldvinsson@netapp.com> Co-authored-by: PythonSdkPipelines <PythonSdkPipelines> Co-authored-by: iscai-msft <43154838+iscai-msft@users.noreply.github.com> Co-authored-by: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com> Co-authored-by: Wes Haggard <Wes.Haggard@microsoft.com> Co-authored-by: Rakshith Bhyravabhotla <sabhyrav@microsoft.com> Co-authored-by: swathipil <76007337+swathipil@users.noreply.github.com> Co-authored-by: McCoy Patiño <39780829+mccoyp@users.noreply.github.com> Co-authored-by: Adam Ling (MSFT) <adam_ling@outlook.com> Co-authored-by: Tong Xu (MSFT) <57166602+v-xuto@users.noreply.github.com> Co-authored-by: ckairen <38804567+ckairen@users.noreply.github.com> Co-authored-by: Charles Lowell <chlowe@microsoft.com> Co-authored-by: Scott Beddall <45376673+scbedd@users.noreply.github.com> Co-authored-by: Azure CLI Bot <azclibot@microsoft.com> Co-authored-by: Jorge Chavez Nieto <jochav@microsoft.com> Co-authored-by: Zed Lei <59104634+RAY-316@users.noreply.github.com> Co-authored-by: Jiefeng Chen <51037443+BigCat20196@users.noreply.github.com> Co-authored-by: Artem Pavlichenko <arpavlic@microsoft.com> Co-authored-by: Krista Pratico <krpratic@microsoft.com> Co-authored-by: Ben Broderick Phillips <bebroder@microsoft.com> Co-authored-by: Mohamed Shaban <v-moshaban@microsoft.com> Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com> Co-authored-by: Xiang Yan <xiangsjtu@gmail.com> Co-authored-by: Chidozie Ononiwu <chononiw@microsoft.com> Co-authored-by: annatisch <antisch@microsoft.com> Co-authored-by: Albert Cheng <albertcheng@microsoft.com>

dev-Sentinel-2022-08-01 (Azure#20290) * Adds base for updating Microsoft.SecurityInsights from version stable/2021-10-01 to version 2022-08-01 * Updates readme * Updates API version in new specs and examples * Add MITRE support to alert rules models (Azure#19757) * Update version * Fix lint diff error Co-authored-by: Lilyan Cohen <licohen@microsoft.com> Co-authored-by: lilyanc02 <46589651+lilyanc02@users.noreply.github.com> Co-authored-by: Lilyan Cohen <licohen@microsoft.com>

iscai-msft added 3 commits August 16, 2021 13:13

switch text from property to method and update existing tests

07407a1

add text and encoding tests

f17bca0

remove internal_response docstrings

2160e4d

iscai-msft requested review from lmazuel, annatisch, xiangyan99 and johanste August 16, 2021 17:36

ghost added the Azure.Core label Aug 16, 2021

xiangyan99 reviewed Aug 16, 2021

View reviewed changes

sdk/core/azure-core/azure/core/rest/_requests_basic.py Outdated Show resolved Hide resolved

iscai-msft added 2 commits August 16, 2021 14:13

remove redundant encoding code

e68f928

update changelog

3541364

iscai-msft marked this pull request as ready for review August 16, 2021 18:15

unify text and encoding accross transports

7e6f83d

iscai-msft commented Aug 19, 2021

View reviewed changes

xiangyan99 reviewed Aug 19, 2021

View reviewed changes

sdk/core/azure-core/azure/core/rest/_rest.py Show resolved Hide resolved

xiangyan99 approved these changes Aug 19, 2021

View reviewed changes

convert to unicode str

bc46726

johanste reviewed Aug 19, 2021

View reviewed changes

iscai-msft added 2 commits August 19, 2021 13:20

remove unnecessary content setting

62a8e5c

remove unnecessary if else statement

59fc8d5

iscai-msft merged commit abfb14b into Azure:main Aug 20, 2021

iscai-msft deleted the test_to_function branch August 20, 2021 00:04

hildurhodd pushed a commit to hildurhodd/azure-sdk-for-python that referenced this pull request Aug 26, 2021

[rest] change text from a property to a method (Azure#20290)

745aeb5

hildurhodd pushed a commit to hildurhodd/azure-sdk-for-python that referenced this pull request Aug 26, 2021

[rest] change text from a property to a method (Azure#20290)

563a406

hildurhodd pushed a commit to hildurhodd/azure-sdk-for-python that referenced this pull request Aug 30, 2021

[rest] change text from a property to a method (Azure#20290)

88c8f7e

iscai-msft added a commit that referenced this pull request Sep 29, 2021

[rest] change text from a property to a method (#20290)

d7ae308

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rest] change text from a property to a method #20290

[rest] change text from a property to a method #20290

iscai-msft commented Aug 16, 2021

iscai-msft Aug 19, 2021

iscai-msft Aug 19, 2021

iscai-msft Aug 19, 2021

xiangyan99 left a comment

iscai-msft commented Aug 19, 2021

azure-pipelines bot commented Aug 19, 2021

johanste Aug 19, 2021

johanste Aug 19, 2021

iscai-msft Aug 19, 2021

johanste Aug 19, 2021

iscai-msft Aug 19, 2021

johanste Aug 19, 2021

iscai-msft Aug 19, 2021

iscai-msft commented Aug 19, 2021

azure-pipelines bot commented Aug 19, 2021

[rest] change text from a property to a method #20290

[rest] change text from a property to a method #20290

Conversation

iscai-msft commented Aug 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangyan99 left a comment

Choose a reason for hiding this comment

iscai-msft commented Aug 19, 2021

azure-pipelines bot commented Aug 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iscai-msft commented Aug 19, 2021

azure-pipelines bot commented Aug 19, 2021