Skip to content

Add typing to basic_checker #6769

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

Merged
merged 1 commit into from
May 31, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2413958046

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0008%) to 95.517%

Totals Coverage Status
Change from base Build 2412991563: 0.0008%
Covered Lines: 16235
Relevant Lines: 16997

πŸ’› - Coveralls

@github-actions
Copy link
Contributor

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on pandas:
The following fatal messages are now emitted: πŸ’£πŸ’₯

  1. astroid-error:
    tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/test_common.py: Fatal error while checking 'tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/test_common.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/runner/.cache/pylint/pylint-crash-2022-05-31-09.txt'.
    Please check your changes on the following file:
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/tests/io/test_common.py#L1
  2. astroid-error:
    tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/sas/test_sas7bdat.py: Fatal error while checking 'tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/sas/test_sas7bdat.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/runner/.cache/pylint/pylint-crash-2022-05-31-09.txt'.
    Please check your changes on the following file:
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/tests/io/sas/test_sas7bdat.py#L1
  3. astroid-error:
    tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/excel/test_readers.py: Fatal error while checking 'tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/excel/test_readers.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/runner/.cache/pylint/pylint-crash-2022-05-31-09.txt'.
    Please check your changes on the following file:
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/tests/io/excel/test_readers.py#L1
  4. astroid-error:
    tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/pytables/test_read.py: Fatal error while checking 'tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/pytables/test_read.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/runner/.cache/pylint/pylint-crash-2022-05-31-09.txt'.
    Please check your changes on the following file:
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/tests/io/pytables/test_read.py#L1

The following messages are now emitted:

  1. docstring-first-line-empty:
    First line empty in function docstring
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/console.py#L78
  2. too-many-try-statements:
    try clause contains 2 statements, expected at most 1
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/console.py#L86
  3. invalid-name:
    Variable name "ip" doesn't conform to '[a-z_][a-z0-9_]{2,30}$' pattern
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/console.py#L88

The following messages are no longer emitted:

  1. astroid-error:
    tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/test_common.py: Fatal error while checking 'tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/test_common.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/runner/.cache/pylint/pylint-crash-2022-05-30-22.txt'.
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/tests/io/test_common.py#L1
  2. astroid-error:
    tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/sas/test_sas7bdat.py: Fatal error while checking 'tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/sas/test_sas7bdat.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/runner/.cache/pylint/pylint-crash-2022-05-30-22.txt'.
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/tests/io/sas/test_sas7bdat.py#L1
  3. astroid-error:
    tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/excel/test_readers.py: Fatal error while checking 'tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/excel/test_readers.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/runner/.cache/pylint/pylint-crash-2022-05-30-22.txt'.
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/tests/io/excel/test_readers.py#L1
  4. astroid-error:
    tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/pytables/test_read.py: Fatal error while checking 'tests/.pylint_primer_tests/pandas-dev/pandas/pandas/tests/io/pytables/test_read.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/runner/.cache/pylint/pylint-crash-2022-05-30-22.txt'.
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/tests/io/pytables/test_read.py#L1
  5. astroid-error:
    tests/.pylint_primer_tests/pandas-dev/pandas/pandas/io/formats/console.py: Fatal error while checking 'tests/.pylint_primer_tests/pandas-dev/pandas/pandas/io/formats/console.py'. Please open an issue in our bug tracker so we address this. There is a pre-filled template that you can use in '/home/runner/.cache/pylint/pylint-crash-2022-05-30-22.txt'.
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/console.py#L1
  6. wrong-import-position:
    Import "from future import annotations" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L4
  7. wrong-import-position:
    Import "from functools import reduce" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L6
  8. wrong-import-position:
    Import "import itertools" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L7
  9. wrong-import-position:
    Import "import re" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L8
  10. wrong-import-position:
    Import "from typing import Any, Callable, Hashable, Iterable, Mapping, Sequence, cast" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L9
  11. wrong-import-position:
    Import "import warnings" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L18
  12. wrong-import-position:
    Import "import numpy as np" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L20
  13. wrong-import-position:
    Import "from pandas._libs.lib import is_list_like" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L22
  14. wrong-import-position:
    Import "from pandas._typing import IndexLabel, StorageOptions" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L23
  15. wrong-import-position:
    Import "from pandas.util._decorators import doc" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L27
  16. wrong-import-position:
    Import "from pandas.core.dtypes import missing" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L29
  17. wrong-import-position:
    Import "from pandas.core.dtypes.common import is_float, is_scalar" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L30
  18. wrong-import-position:
    Import "from pandas import DataFrame, Index, MultiIndex, PeriodIndex" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L35
  19. wrong-import-position:
    Import "import pandas.core.common as com" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L41
  20. wrong-import-position:
    Import "from pandas.core.shared_docs import _shared_docs" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L42
  21. wrong-import-position:
    Import "from pandas.io.formats._color_data import CSS4_COLORS" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L44
  22. wrong-import-position:
    Import "from pandas.io.formats.css import CSSResolver, CSSWarning" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L45
  23. wrong-import-position:
    Import "from pandas.io.formats.format import get_level_lengths" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L49
  24. wrong-import-position:
    Import "from pandas.io.formats.printing import pprint_thing" should be placed at the top of the module
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/io/formats/excel.py#L50

@Pierre-Sassoulas
Copy link
Member

Is the primer broken because we're on bleeding edge astroid ? I feel like it should be said somewhere if that's the case. Also we probably want to provide the crash template file instead of the pylint message ?

@DanielNoord
Copy link
Collaborator Author

Is the primer broken because we're on bleeding edge astroid ?

@jacobtylerwalls Another importlib issue.. Do you recognise it?

I feel like it should be said somewhere if that's the case. Also we probably want to provide the crash template file instead of the pylint message ?

I don't think we can easily extract the crash template from the environment. Perhaps we can redirect stderr to a file and print it so you at least get the stacktrace.

@DanielNoord DanielNoord merged commit 4295d41 into pylint-dev:main May 31, 2022
@DanielNoord DanielNoord deleted the typing-checkers-1 branch May 31, 2022 11:22
@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented May 31, 2022

It was fixed in the Astroid PR we merged this morning. The job did not fail, just extraneous comments. It's the "race condition" of one unlucky contributor pushing right after an Astroid change involving a crash and right before a pylint push to main. We are testing an incredible amount of code against astroid now with the primer, I don't expect this a lot!

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented May 31, 2022

Also we probably want to provide the crash template file instead of the pylint message ?

All of it in the log, including a GitHub Actions warning to point folks directly to it. We did a lot of work around this :D

I agree it could still surprise a contributor. I'm realizing it's not just astroid -- we've solved the failing jobs issue from using bleeding edge. It could be any of the open source projects in the primer that push a new commit, we then have a drift in messages emitted between main and PR, no? I'll post on the other thread.

@Pierre-Sassoulas
Copy link
Member

It could be any of the open source projects in the primer that push a new commit, we then have a drift in messages emitted between main and PR, no? I'll post on the other thread.

I think we discussed this with @DanielNoord when we added the original primer, there's a system in place for caching based on a concatenation of all repo hashes.

@DanielNoord
Copy link
Collaborator Author

@jacobtylerwalls I think I found the issue. It's a funny one (for us developers that is...)

It can't be the external project pushing a commit. Luckily I though of that, which is why only the main job can download and cache the packages. That way we make sure packages stay consistent across all runs. You can check this system is working in the run: https://github.com/PyCQA/pylint/runs/6667771616?check_suite_focus=true. In step Download last 'main' run info we print out the last main run we are using. In Check cache we print out the commits of all packages so they can be compared and the working of the cache can be confirmed. Everything seems in order there.

However, notice which run this PR chose to use:
https://github.com/PyCQA/pylint/actions/runs/2411650282

That's a run of 13 hours ago based on this commit: ceab9aa
pylint-dev/astroid@1ccb1c4 was only merged 4 hours ago, so it is expected that we have some failures.

Why are we using such on old commit while there were more recent commits? https://github.com/PyCQA/pylint/commits/main > 9935d49

Because I'm dumb and ignore any changes to doc/data/messages/** in https://github.com/PyCQA/pylint/blob/main/.github/workflows/primer_run_main.yaml. Thus, whenever we have a commit that only changes the messages documentation we don't actually run Primer / Main. So we didn't actually have any Primer / Main job after that astroid fix was pushed 4 hours ago.

TLDR: Let's run Primer / Main on all commits to main and remove the paths-ignore. I think it's good to have a run as recent as possible.

@Pierre-Sassoulas
Copy link
Member

I found the issue. It's a funny one (for us developers that is...)

That is some back-of-the-labtop-worthy quote. (Also, great analysis πŸ‘ )

@jacobtylerwalls
Copy link
Member

Because I'm dumb and ignore any changes to doc/data/messages/** in

I was the one that asked for this, sorry! 🧐

@DanielNoord
Copy link
Collaborator Author

Because I'm dumb and ignore any changes to doc/data/messages/** in

I was the one that asked for this, sorry! 🧐

Oh really? Oh well, then I should have though about the impact better πŸ˜„ I'm very happy I decided to develop this on pylint instead of a different repository. Having to release new versions for all these bug fixed would have been awful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants