Skip to content

Use context managers for disable_errors() #10251

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

Closed
wants to merge 8 commits into from
Closed

Conversation

dixith
Copy link
Contributor

@dixith dixith commented Mar 27, 2021

Description

This PR fixes one of the issue reported in #1184

Test Plan

Since this change is coding style change, new cases are not added.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

mypy/messages.py Outdated
self.disable_count += 1
yield
Copy link
Member

Choose a reason for hiding this comment

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

Should use try-finally

Copy link
Contributor Author

@dixith dixith Mar 27, 2021

Choose a reason for hiding this comment

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

Thank you for the review @JelleZijlstra

yield should be inside try-finally?

@contextmanager
def disable_errors(self) -> Iterator[None]:
    self.disable_count += 1
    try:
        yield
    finally:
        pass
    self.disable_count -= 1

Copy link
Member

Choose a reason for hiding this comment

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

And the -= 1 should be in the finally block. The point is to make sure the cleanup code always happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JelleZijlstra Now that we have try-finally inside enable_errors() below try-finally is redundent right? Can be removed?

right_type = self.analyze_cond_branch(right_map, e.right, left_type)

if right_map is None:
  with self.msg.disable_errors():
     try:
        right_type = self.analyze_cond_branch(right_map, e.right, left_type)
     finally:
        pass

Copy link
Member

Choose a reason for hiding this comment

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

A try-finally with an empty finally block doesn't do anything useful, so yes you can remove it.

self.disable_count += 1
yield
self.disable_count -= 1

def enable_errors(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Can this method be removed?

Copy link
Contributor Author

@dixith dixith Mar 27, 2021

Choose a reason for hiding this comment

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

Removed enable_errors()

@dixith
Copy link
Contributor Author

dixith commented Mar 27, 2021

Getting below error.
mypy/checkexpr.py:2772: error: Local variable 'right_map' has inferred type None; add an annotation

@dixith
Copy link
Contributor Author

dixith commented Apr 2, 2021

@JelleZijlstra

Getting below error. Do you ahve any suggestions?
mypy/checkexpr.py:2772: error: Local variable 'right_map' has inferred type None; add an annotation

@JelleZijlstra
Copy link
Member

Sorry, I'm not too familiar with that code. I'd try adding an annotation though, as the error message suggests.

@dixith dixith force-pushed the master branch 2 times, most recently from 58c4ded to c7a2b5c Compare April 3, 2021 07:13
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

flake8 (https://github.com/pycqa/flake8.git)
+ src/flake8/checker.py:471: error: "Optional[FileProcessor]" has no attribute "build_ast"
+ src/flake8/checker.py:497: error: "Optional[FileProcessor]" has no attribute "build_logical_line"
+ src/flake8/checker.py:500: error: "Optional[FileProcessor]" has no attribute "update_state"
+ src/flake8/checker.py:505: error: "Optional[FileProcessor]" has no attribute "update_checker_state_for"
+ src/flake8/checker.py:518: error: "Optional[FileProcessor]" has no attribute "next_logical_line"
+ src/flake8/checker.py:526: error: "Optional[FileProcessor]" has no attribute "update_checker_state_for"
+ src/flake8/checker.py:561: error: "Optional[FileProcessor]" has no attribute "generate_tokens"
+ src/flake8/checker.py:599: error: "Optional[FileProcessor]" has no attribute "reset_blank_before"
+ src/flake8/checker.py:602: error: "Optional[FileProcessor]" has no attribute "visited_new_blank_line"
+ src/flake8/checker.py:603: error: "Optional[FileProcessor]" has no attribute "delete_first_token"
+ src/flake8/checker.py:633: error: "Optional[FileProcessor]" has no attribute "inside_multiline"
+ src/flake8/checker.py:634: error: "Optional[FileProcessor]" has no attribute "split_line"

pydantic (https://github.com/samuelcolvin/pydantic.git)
+ pydantic/validators.py:195: error: Unsupported operand types for < ("int" and "None")  [operator]
+ pydantic/validators.py:199: error: Unsupported operand types for > ("int" and "None")  [operator]

prefect (https://github.com/PrefectHQ/prefect.git)
+ src/prefect/environments/execution/dask/cloud_provider.py:140: error: "None" has no attribute "scheduler"


pandas (https://github.com/pandas-dev/pandas.git)
+ pandas/io/html.py:1092: error: Unsupported operand types for > ("int" and "None")  [operator]
+ pandas/io/html.py:1092: error: Unsupported operand types for > ("int" and "Sequence[int]")  [operator]
+ pandas/io/html.py:1092: error: Unsupported operand types for > ("int" and "slice")  [operator]
+ pandas/io/html.py:1092: note: Left operand is of type "Union[int, Sequence[int], slice, None]"
+ pandas/tseries/holiday.py:451: error: Value of type "None" is not indexable  [index]

aioredis (https://github.com/aio-libs/aioredis.git)
+ aioredis/client.py:3774: error: "Optional[Connection]" has no attribute "read_response"
+ aioredis/client.py:3779: error: "Optional[Match[str]]" has no attribute "groups"


tornado (https://github.com/tornadoweb/tornado.git)
+ tornado/web.py:1715: error: "None" has no attribute "done"

poetry (https://github.com/python-poetry/poetry.git)
+ poetry/utils/appdirs.py:27: error: "Union[str, Path]" has no attribute "startswith"
+ poetry/utils/env.py:1236: error: "Optional[Command]" has no attribute "finalize_options"
+ poetry/utils/env.py:1310: error: "Union[str, int]" has no attribute "strip"
+ poetry/utils/env.py:1321: error: "Union[str, int]" has no attribute "strip"
+ poetry/utils/env.py:1367: error: "Union[int, str]" has no attribute "strip"
+ poetry/version/version_selector.py:54: error: "None" has no attribute "version"
+ poetry/publishing/uploader.py:157: error: "Optional[Match[str]]" has no attribute "group"

spark (https://github.com/apache/spark.git)
+ python/pyspark/shuffle.py:602: error: "Optional[Any]" has no attribute "tell"
+ python/pyspark/shuffle.py:606: error: "Optional[Any]" has no attribute "tell"

pytest (https://github.com/pytest-dev/pytest.git)
+ src/_pytest/fixtures.py:1107: error: "Union[Callable[..., FixtureValue], Callable[..., Generator[FixtureValue, None, None]]]" has no attribute "__get__"  [attr-defined]
+ src/_pytest/fixtures.py:1121: error: "Union[Callable[..., FixtureValue], Callable[..., Generator[FixtureValue, None, None]]]" has no attribute "__get__"  [attr-defined]

isort (https://github.com/pycqa/isort.git)
+ isort/_vendored/toml/decoder.py:888: error: "Optional[Match[str]]" has no attribute "groups"

graphql-core (https://github.com/graphql-python/graphql-core.git)
+ src/graphql/language/block_string.py:52: error: Unsupported operand types for < ("int" and "None")

@dixith dixith force-pushed the master branch 4 times, most recently from 587937c to 717e7ff Compare April 3, 2021 08:04
@dixith dixith marked this pull request as draft April 4, 2021 11:17
@JelleZijlstra
Copy link
Member

Sorry for dropping this! We ended up merging #10569 instead.

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.

2 participants