Skip to content

gh-104683: Argument Clinic: Modernise parse_special_symbol() - #106837

Merged
erlend-aasland merged 8 commits into
python:mainfrom
erlend-aasland:clinic/modernise-parse-special-symbol
Jul 17, 2023
Merged

gh-104683: Argument Clinic: Modernise parse_special_symbol()#106837
erlend-aasland merged 8 commits into
python:mainfrom
erlend-aasland:clinic/modernise-parse-special-symbol

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented Jul 17, 2023•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@bedevere-botbedevere-bot mentioned this pull request Jul 17, 2023
Closed
7 tasks
@erlend-aasland

erlend-aasland commented Jul 17, 2023

Copy link
Copy Markdown
ContributorAuthor

@AlexWaygood what do you think of this? Is it worth it?

AlexWaygood
AlexWaygood reviewed Jul 17, 2023
View reviewed changes

@AlexWaygoodAlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Haven't done a thorough review yet, just skimmed, but I think this is a great idea. It makes it much more readable imo, and could also make it easier to create dedicated unit tests for each method

Comment threadTools/clinic/clinic.py Outdated
erlend-aasland
erlend-aasland commented Jul 17, 2023
View reviewed changes
Comment threadTools/clinic/clinic.py Outdated
@erlend-aasland

erlend-aasland commented Jul 17, 2023

Copy link
Copy Markdown
ContributorAuthor

Haven't done a thorough review yet, just skimmed, but I think this is a great idea. It makes it much more readable imo, and could also make it easier to create dedicated unit tests for each method

Thanks for the preliminary review, though :) BTW, test coverage is already pretty good for the affected lines of code.

@erlend-aasland
erlend-aasland marked this pull request as ready for review July 17, 2023 21:04
AlexWaygood
AlexWaygood reviewed Jul 17, 2023
View reviewed changes
Comment threadTools/clinic/clinic.py Outdated
Comment threadTools/clinic/clinic.py Outdated
Comment threadTools/clinic/clinic.py Outdated
@erlend-aasland

erlend-aasland commented Jul 17, 2023

Copy link
Copy Markdown
ContributorAuthor

Latest remarks addressed in deb05a2. Thanks!

AlexWaygood
AlexWaygood approved these changes Jul 17, 2023
View reviewed changes

@AlexWaygoodAlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a great improvement. Thanks!

Comment threadTools/clinic/clinic.py Outdated
AlexWaygood
AlexWaygood reviewed Jul 17, 2023
View reviewed changes
Comment threadTools/clinic/clinic.py Outdated
@erlend-aasland@AlexWaygood
Apply suggestions from code review
b0ae589
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@erlend-aasland
erlend-aasland enabled auto-merge (squash) July 17, 2023 21:46
@erlend-aasland
erlend-aasland merged commit 00e52ac into python:mainJul 17, 2023
@erlend-aasland
erlend-aasland deleted the clinic/modernise-parse-special-symbol branch August 22, 2023 08:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sobolevnsobolevnsobolevn left review comments
@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland@sobolevn@AlexWaygood@bedevere-bot