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

[SIM906] Merge nested os.path.join calls #101

Closed
Skylion007 opened this issue Feb 16, 2022 · 5 comments · Fixed by #104
Closed

[SIM906] Merge nested os.path.join calls #101

Skylion007 opened this issue Feb 16, 2022 · 5 comments · Fixed by #104
Assignees
Labels
enhancement New feature or request

Comments

@Skylion007
Copy link
Contributor

Explanation

Explain briefly why you think this makes the code simpler.

  • This is a common but fun one I have noticed. I have noticed this pattern occur a lot and I believe in most cases it can be simplified to a single function call. I guess this would be kinda of similar to the isinstance checks where people don't realize the function can consume multiple args.

Example

a = "/a"
b = "b"
c = "c"
# Bad
os.path.join(a,os.path.join(b,c))

# Good
os.path.join(a,b,c)
@Skylion007 Skylion007 added the enhancement New feature or request label Feb 16, 2022
@MartinThoma
Copy link
Owner

I didn't know that this was possible! I'll add this when I have more time for sure - this should also be easy / have little potential for false-positives. Very nice suggestion!

@MartinThoma
Copy link
Owner

Side-note: pathlib.Path should be used more often. If a, b, and c were Path objects, you could simply do:

d = a / b / c

@MartinThoma
Copy link
Owner

Many usages of os.path can be replaced by pathlib.

@Skylion007
Copy link
Contributor Author

@MartinThoma True, but there can be performance issues with using Pathlib. For instance, I think the black formatter removed / reworked a lot of their Pathlib usage back to os.path in their library due to performance issues.

@MartinThoma
Copy link
Owner

$ astpretty --no-show-offsets /dev/stdin <<< `cat example.txt`
Module(
    body=[
        Expr(
            value=Call(
                func=Attribute(
                    value=Attribute(
                        value=Name(id='os', ctx=Load()),
                        attr='path',
                        ctx=Load(),
                    ),
                    attr='join',
                    ctx=Load(),
                ),
                args=[
                    Name(id='a', ctx=Load()),
                    Call(
                        func=Attribute(
                            value=Attribute(
                                value=Name(id='os', ctx=Load()),
                                attr='path',
                                ctx=Load(),
                            ),
                            attr='join',
                            ctx=Load(),
                        ),
                        args=[
                            Name(id='b', ctx=Load()),
                            Name(id='c', ctx=Load()),
                        ],
                        keywords=[],
                    ),
                ],
                keywords=[],
            ),
        ),
    ],
    type_ignores=[],
)

@MartinThoma MartinThoma changed the title [New Rule] os.path.join() call simplification [SIM906] os.path.join() call simplification Feb 20, 2022
@MartinThoma MartinThoma changed the title [SIM906] os.path.join() call simplification [SIM906] Merge nested os.path.join calls Feb 20, 2022
MartinThoma added a commit that referenced this issue Feb 20, 2022
Credits to Skylion007 for defining this rule!

Closes #101
MartinThoma added a commit that referenced this issue Feb 20, 2022
Credits to Skylion007 for defining this rule!

Closes #101
MartinThoma added a commit that referenced this issue Feb 20, 2022
Credits to Skylion007 for defining this rule!

Closes #101
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants