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

[doc] Add a check for changelogs and fix the issues encountered #6735

Merged
merged 7 commits into from
Jun 1, 2022

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
βœ“ πŸ”¨ Refactoring
βœ“ πŸ“œ Docs

Description

Refs #6688

@@ -658,7 +658,7 @@ Release date: 2006-08-10
* started a reference user manual
Copy link
Member Author

Choose a reason for hiding this comment

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

Not touching this file (unless it's a global sed) because it's a mix between Logilab's internal and bitbucket. Hard to tell, probably dead link anyway, and not very useful generally.

@coveralls
Copy link

coveralls commented May 29, 2022

Pull Request Test Coverage Report for Build 2419895913

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.516%

Totals Coverage Status
Change from base Build 2417473648: 0.0%
Covered Lines: 16231
Relevant Lines: 16993

πŸ’› - Coveralls

from pathlib import Path
from re import Pattern

ISSUE_NUMBER_PATTERN = re.compile(r"#\d{1,5}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we reuse this pattern in the other patterns?

Suggested change
ISSUE_NUMBER_PATTERN = re.compile(r"#\d{1,5}")
ISSUE_NUMBER_PATTERN = re.compile(r"#\d+")

Copy link
Member Author

Choose a reason for hiding this comment

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

I see you're an optimist πŸ˜„

Copy link
Member Author

Choose a reason for hiding this comment

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

But I think it's a good test to keep 5, if we have a ticket going above 99999 in pylint it(s probably wrong. (Logilab's issue are above 100k for example.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still, let's reuse the pattern so if we ever need to change it that's just in one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 9a60d73 with some bonus modularity

script/check_changelog.py Show resolved Hide resolved
doc/user_guide/checkers/features.rst Outdated Show resolved Hide resolved
@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-12.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-12.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-12.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-12.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 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-31-11.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-31-11.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-31-11.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-31-11.txt'.
    https://github.com/pandas-dev/pandas.git/blob/main/pandas/tests/io/pytables/test_read.py#L1

]

NO_CHECK_REQUIRED_FILES = {
"index.rst",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't these be excluded in pre-commit? Feels a bit weird to exclude them here.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a function to select the file we want to check (sorted_whatsnew) and we're launching on the whole repository. The function has verbose log to troubleshoot, I'd rather keep that than to have to use an elaborate exclude regex. Also later on we could want to check that it's in the "right" file according to pylint's version and we need to have information about the whole repo and the other files to do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, doesn't it make more sense to let this accept a files argument? That's how most of the linting, formatting, etc. tools work

Copy link
Member Author

Choose a reason for hiding this comment

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

It does the job, we won't use it that much (when we modify the old changelog), it's not perfect but I don't see the point in refactoring it to perfection right now as it's blocking 2.14. Let's refactor it if we're going to keep the

* blabla

  Closes #5445345

format in #6688 (I.e. after 2.14). Right now I think * blabla (#123456789) with 123456789 being the PR number and not the issue number would be better, which means we would delete this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also see #6781, if we auto-generate the changelog we won't need it either

@Pierre-Sassoulas Pierre-Sassoulas merged commit 214201a into pylint-dev:main Jun 1, 2022
@Pierre-Sassoulas Pierre-Sassoulas deleted the changelog-check branch June 1, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants