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

Accept math constants in defaults #355

Merged
merged 11 commits into from
Mar 13, 2023
Merged

Conversation

XuehaiPan
Copy link
Contributor

@XuehaiPan XuehaiPan commented Mar 12, 2023

Allow math.inf, math.e, math.pi, math.tau, and their negatives in defaults.

Reject:

  • float('inf') / float('-inf'): function calls
  • math.nan: not constant math.nan == math.nan -> False

Resolves #354

@github-actions

This comment has been minimized.

Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

I don't think it needs to be nearly this complicated, though. If you just add these constants to the _ALLOWED_ATTRIBUTES_IN_DEFAULTS frozenset, I don't think you'll need to make any other changes to pyi.py

@XuehaiPan
Copy link
Contributor Author

I don't think it needs to be nearly this complicated, though.

I think -math.inf should be allowed here.

$ echo 'import math; x = -math.inf' | python3 -m ast
Module(
   body=[
      Import(
         names=[
            alias(name='math')]),
      Assign(
         targets=[
            Name(id='x', ctx=Store())],
         value=UnaryOp(
            op=USub(),
            operand=Attribute(
               value=Name(id='math', ctx=Load()),
               attr='inf',
               ctx=Load())))],
   type_ignores=[])

The AST for -math.inf is UnaryOp.

@AlexWaygood
Copy link
Collaborator

AlexWaygood commented Mar 12, 2023

Ah, I see. That requires a slightly more invasive change to the code than I was expecting, but I suppose it's reasonable.

pyi.py Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Collaborator

Could you add a CHANGELOG entry? This file here: https://github.com/PyCQA/flake8-pyi/blob/main/CHANGELOG.md

CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

A few changelog nitpicks :)

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
XuehaiPan and others added 2 commits March 13, 2023 01:46
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
pyi.py Outdated
@@ -695,6 +695,10 @@ def _analyse_union(members: Sequence[ast.expr]) -> UnionAnalysis:
)


_ALLOWED_MATH_ATTRIBUTES_IN_DEFAULTS = frozenset(
{"math.inf", "math.e", "math.pi", "math.tau"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This set is small enough that we could probably actually just inline it in the _is_valid_math_constant function. But on the other hand, I guess having it as a global constant like this maybe makes the code more extensible, if we want to add this to this set in the future. No strong opinion either way.

Copy link
Contributor Author

@XuehaiPan XuehaiPan Mar 12, 2023

Choose a reason for hiding this comment

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

Since the function will be called multiple times, caching the frozenset as a global variable will somehow gain some speed. And also, it would be extensible.

Copy link
Collaborator

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll wait a little bit before merging in case any other maintainers have objections :)

@github-actions

This comment has been minimized.

pyi.py Outdated Show resolved Hide resolved
@github-actions
Copy link

This change has no effect on typeshed. 🤖🎉

@AlexWaygood AlexWaygood merged commit 5cb5b15 into PyCQA:main Mar 13, 2023
@AlexWaygood
Copy link
Collaborator

Thanks @XuehaiPan!

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.

Y011: Allow math.inf, math.e, math.pi, and math.tau as simple defaults
3 participants