Skip to content

Conversation

@codeboten
Copy link
Contributor

Description

Continuing work to support pylint 2.11.0, addressing string formatting in code.

Fixes #692

@codeboten codeboten added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Sep 21, 2021
@codeboten codeboten requested a review from a team September 21, 2021 18:26
@github-actions github-actions bot requested review from NathanielRN and removed request for a team September 21, 2021 18:27
)
self.assertEqual(
response.headers["traceresponse"],
"00-{0}-{1}-01".format(
Copy link
Member

@aabmass aabmass Sep 23, 2021

Choose a reason for hiding this comment

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

Cases like these where you don't have short variable, f-strings can be pretty ugly :( imo it's pretty unreadable compared to the format equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hear that... I don't feel strongly either ways. I found the formatting previously there confusing as well w/ the multiple 0's and 1's 😄

Could create a temporary variable or just disable pylint for this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably more readable when it's not complex code buried in a string - I prefer the format one.

Copy link
Member

Choose a reason for hiding this comment

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

I found the formatting previously there confusing as well w/ the multiple 0's and 1's

I agree, maybe short variable names with f-strings would be better here, or could do "00-{}-{}-01".format(...) or named format params. I'll leave it up to you, don't want to block this @codeboten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm happy to fix this in a following PR. I will refactor all the tests that are duplicated into a single test in the base class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will use the following method to simplify the code open-telemetry/opentelemetry-python#2159

@owais owais enabled auto-merge (squash) September 27, 2021 23:50
@owais owais merged commit fbb677a into open-telemetry:main Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use f-strings wherever possible

7 participants