Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

isinstance checks in os.path.normcase redundant with os.fspath #74612

Closed
wm75 mannequin opened this issue May 22, 2017 · 7 comments
Closed

isinstance checks in os.path.normcase redundant with os.fspath #74612

wm75 mannequin opened this issue May 22, 2017 · 7 comments
Labels
3.7 (EOL) end of life performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@wm75
Copy link
Mannequin

wm75 mannequin commented May 22, 2017

BPO 30427
Nosy @brettcannon, @serhiy-storchaka, @wm75, @miss-islington, @isidentical
PRs
  • bpo-30427: eliminate redundant type checks in os.path.normcase() #1712
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2020-01-02.22:13:44.727>
created_at = <Date 2017-05-22.08:17:57.051>
labels = ['3.7', 'library', 'performance']
title = 'isinstance checks in os.path.normcase redundant with os.fspath'
updated_at = <Date 2020-01-02.22:13:44.726>
user = 'https://github.com/wm75'

    bugs.python.org fields:

    activity = <Date 2020-01-02.22:13:44.726>
actor = 'brett.cannon'
assignee = 'none'
closed = True
closed_date = <Date 2020-01-02.22:13:44.727>
closer = 'brett.cannon'
components = ['Library (Lib)']
creation = <Date 2017-05-22.08:17:57.051>
creator = 'wolma'
dependencies = []
files = []
hgrepos = []
issue_num = 30427
keywords = []
message_count = 7.0
messages = ['294130', '294131', '294137', '294206', '339077', '358868', '359223']
nosy_count = 5.0
nosy_names = ['brett.cannon', 'serhiy.storchaka', 'wolma', 'miss-islington', 'BTaskaya']
pr_nums = ['1712']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue30427'
versions = ['Python 3.6', 'Python 3.7']
    @wm75
    Copy link
    Mannequin Author

    wm75 mannequin commented May 22, 2017

    os.path.normcase as defined in both posixpath and ntpath is now calling os.fspath on its argument first. With that I think the following isinstance(str, bytes) checks have become redundant since AFAIU os.fspath is guaranteed to return either str or bytes instances.

    @wm75 wm75 mannequin added 3.7 (EOL) end of life stdlib Python modules in the Lib dir labels May 22, 2017
    @wm75
    Copy link
    Mannequin Author

    wm75 mannequin commented May 22, 2017

    Just created a PR for this, which eliminates the redundancy. This also changes the error message (making it less specific), but not the type of a raised exception.
    If you think that the error message deserves to be preserved that could, of course, be done too.

    @wm75 wm75 mannequin added the performance Performance or resource usage label May 22, 2017
    @serhiy-storchaka
    Copy link
    Member

    Does calling os.fspath() needed in normcase() at all? Is normcase() even called with arguments which are not just str/bytes?

    @brettcannon
    Copy link
    Member

    The problem with leaving os.fspath() out of os.path.normcase() is that suddenly a single function that deals with paths won't work with path-like objects. So that means support for path-like objects won't implicitly work in code that assumes a path but doesn't explicitly go out of its way to support path-like objects.

    @miss-islington
    Copy link
    Contributor

    New changeset 74510e2 by Miss Islington (bot) (Wolfgang Maier) in branch 'master':
    bpo-30427: eliminate redundant type checks in os.path.normcase() (GH-1712)
    74510e2

    @isidentical
    Copy link
    Sponsor Member

    Looks like there is nothing left after PR 1712 if I am not missing something, can this issue be closed?

    @brettcannon
    Copy link
    Member

    Yep, it looks like this is fixed, Batuhan. Thanks for letting us know!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @brettcannon @serhiy-storchaka @miss-islington @isidentical