Skip to content

Report files in original (filesystem) case#5642

Merged
pfmoore merged 2 commits intopypa:masterfrom
pfmoore:correct_case
Jul 23, 2018
Merged

Report files in original (filesystem) case#5642
pfmoore merged 2 commits intopypa:masterfrom
pfmoore:correct_case

Conversation

@pfmoore
Copy link
Copy Markdown
Member

@pfmoore pfmoore commented Jul 23, 2018

PR based on #4770 (comment)

@pfmoore pfmoore added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jul 23, 2018
Copy link
Copy Markdown
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

LGTM.

Flake8 doesn't seem happy about the conditional. Maybe move out the condition into parentheses and a variable?

@pfmoore
Copy link
Copy Markdown
Member Author

pfmoore commented Jul 23, 2018

@pradyunsg Yeah flake8 is a whiny little nuisance. I'm fixing it now.

@pfmoore
Copy link
Copy Markdown
Member Author

pfmoore commented Jul 23, 2018

Fixed it by making the code look uglier :-(

file_ = os.path.normcase(os.path.join(dirpath, fname))
if os.path.isfile(file_) and file_ not in files:
file_ = os.path.join(dirpath, fname)
if (os.path.isfile(file_) and
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.

How about making a should_skip variable here? That way the conditional can be more readable. :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Assigning a should_skip variable would still have to go across two lines, so no real readability improvement. TBH, I can't be bothered - this is a case where arguing about readbility improvements is non-productive IMO, so I'd rather just leave it.

At the root, I just disagree with flake8's rule here, so closing my eyes and doing something that conforms to the rule is sufficient for me.

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.

Cool. I don't think it's super important anyway. :P

@pfmoore pfmoore merged commit 8cc943e into pypa:master Jul 23, 2018
@pfmoore pfmoore deleted the correct_case branch July 23, 2018 14:09
@lock
Copy link
Copy Markdown

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants