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

Remove unnecessarily complicated backwards compatibility code #408

Closed
wants to merge 4 commits into from

Conversation

kimvais
Copy link

@kimvais kimvais commented Sep 15, 2023

asyncio.iscoroutinefunction would be needed as a fallback only for Python 3.4 as 3.3 didn't have asyncio and 3.5 introduced inspect.iscoroutinefunction

Also made the 3.12 version check explicit instead of implicit based on the existence of the decorator.

@andrewgodwin
Copy link
Member

What do you see as the advantage of making the version check based on the number rather than the attribute existing? I'd rather do it on the presence of the attribute in case something like PyPy hasn't implemented all of a new inspect yet.

@kimvais
Copy link
Author

kimvais commented Sep 15, 2023

Well, I'm a strong believer that "Readability counts" is the most important piece of PEP 20 and in my opinion making any code less consistent (and therefore arguably less readable) because some third-party static analysis tool might not be up to speed is counterproductive.

But fair enough, no objection on changing the if clause back to what it was. I guess I got a bit distracted by the bit confusing comment :)

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

This looks OK.

Folks have been using, and documented examples point to, the asyncio import rather than the inspect one, so there might be a head scratch here, but I don't suppose it matters. Current advice is "use the shim until 3.12 is minimum version". There's no reason for folks to open the box.

(Ref the previous discussion, feature detection is better than a hard version check, one might think, but that's been resolved.)

asgiref/sync.py Outdated Show resolved Hide resolved
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Sorry, I made a mistake previously, only looking at the code. (Never speak before the CI :)

We can't make this change. asyncio.iscoroutinefunction() is not equivalent to inspect.iscoroutinefunction() prior to Python 3.12, because it performs the additional check for the _is_coroutine marker.

That's why the tests are failing here. (I ran it against Django's test suite to check, and that revealed the same problem. 🤦‍♀️)

To be honest, I think the comment as written has needed information.

@kimvais kimvais closed this Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants