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

SIM401 should not apply to non-concrete default values #1809

Closed
thomasdesr opened this issue Jan 12, 2023 · 6 comments · Fixed by #1825
Closed

SIM401 should not apply to non-concrete default values #1809

thomasdesr opened this issue Jan 12, 2023 · 6 comments · Fixed by #1825
Assignees
Labels
bug Something isn't working

Comments

@thomasdesr
Copy link

thomasdesr commented Jan 12, 2023

Thanks again for Ruff ❤️

As of ruff==0.0.219 SIM401 seems to be incorrectly applied when the default value is not a constant value.

import asyncio


async def some_async_func():
    await asyncio.sleep(10)


async def foo():
    d = {"key": "value"}
    key = "key"

    if key in d:
        value = d[key]
    else:
        value = await some_async_func()

    return value

Believes it can be simplified (& maybe more dangerously will be auto-fixed) to:

import asyncio


async def some_async_func():
    await asyncio.sleep(10)


async def foo():
    d = {"key": "value"}
    key = "key"

    value = d.get(key, await some_async_func())

    return value

However these two are not interchangeable as some_async_func() may be arbitrarily expensive as demonstrated here by the asyncio.sleep(10)

Note: This is also a bug in upstream flake8-simplify MartinThoma/flake8-simplify#166

@charliermarsh
Copy link
Member

Ahh, good catch! We can fix this.

@thomasdesr
Copy link
Author

Thanks!

@charliermarsh
Copy link
Member

I went off some heuristics (ignore any expressions containing function calls, awaits, etc.) — let me know if it misses anything going forward.

@thomasdesr
Copy link
Author

thomasdesr commented Jan 13, 2023

They look like sufficient for my code base! Thanks so much :D

@andersk
Copy link
Contributor

andersk commented Jan 18, 2023

We should also ignore subscripting, since it might throw KeyError. For example, this fix is wrong:

-             if "title" in fileinfo: 
-                 file_name = fileinfo["title"] 
-             else: 
-                 file_name = fileinfo["name"] 
+             file_name = fileinfo.get("title", fileinfo["name"])

@charliermarsh
Copy link
Member

Makes sense, I can fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants