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

Improve wording on W0402 message #6387

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

omarandlorraine
Copy link
Contributor

Type of Changes

Type
📜 Docs

Description

I left the scrolly thing up there since the Issue has the Documentation tag, hope that's the right thing to do.

So I have improved the wording on W0402; it now says "Deprecated module <module name>" for consistency and style.

Closes #6169

@omarandlorraine
Copy link
Contributor Author

omarandlorraine commented Apr 19, 2022

Oh pants!

I think commit f4d4107 should not be part of this branch. Should I try and rewrite history or maybe it doesn't matter?

(the commit in question belongs with PR #6280)

(Okay, I just went ahead and reverted the duff commit. Maybe a rebase could get rid of the commit and the revert?)

@omarandlorraine
Copy link
Contributor Author

Does anyone know what rstcheck and mypy means and why it's failing the precommit hook?

@omarandlorraine omarandlorraine changed the title W0402polish Improve wording on W0402 message Apr 19, 2022
@omarandlorraine
Copy link
Contributor Author

Oh, magically rstcheck passes since I reverted f4d4107. Maybe rstcheck is a changelog sanity checker, who knows.

But mypy still seems to bomb out with some more serious-looking errors.

@Pierre-Sassoulas
Copy link
Member

Do not take the current pre-commit fail into account, our main branch is broken we'll fix it :)

ChangeLog Outdated Show resolved Hide resolved
@DudeNr33
Copy link
Collaborator

Does anyone know what rstcheck and mypy means and why it's failing the precommit hook?

rstcheck is a syntax checker for reStructuredText files. It checks all *.rst files as well as Changelog (which is written in reStructuredText as well).
mypy is an optional static type checker for Python and used to check type conflicts in the code base.

I encourage you to install and activate pre-commit in your own development environment according to our docs.
This will run all configured checks against the staged changes when you run git commit.
This not only allows you to fix issues before you push your branch, but also makes it a bit more transparent what the problems are, as you see the tool output directly on your command line.
Last but not least you will quickly learn what the style checks etc. for pylint expect, and you can write code and documentation directly in a way that the pre-commit hooks won't complain. :)

@omarandlorraine
Copy link
Contributor Author

I encourage you to install and activate pre-commit in your own development environment according to our docs.

Oh, that's a great idea and works really well, thanks

Copy link
Contributor Author

@omarandlorraine omarandlorraine left a comment

Choose a reason for hiding this comment

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

From what I see, this review needs to be concluded or something before the remaining checks will run.

Sorry if I'm generating a lot of spam, I don't know github that well yet.

@Pierre-Sassoulas
Copy link
Member

From what I see, this review needs to be concluded or something before the remaining checks will run.

To prevent bitcoin mining first time contributors need to be approved before the pipeline runs :) You won't have to be approved after this one get merged !

@omarandlorraine
Copy link
Contributor Author

Can anyone see if the remaining tests are failing because of an issue with the PR or because of that problem with the main branch?

The precommit passes locally on my machine but there is a "Some checks were not successful" on github and I don't understand. It is worth noting that here I have python 3.9.2 only, and this is presumably why the tests for other versions are not being run.

My working tree is clean and up to date with origin/W0402polish, (origin being my github repo.) and git grep -i "uses of a deprecated" returns nothing.

@DanielNoord
Copy link
Collaborator

Can anyone see if the remaining tests are failing because of an issue with the PR or because of that problem with the main branch?

The precommit passes locally on my machine but there is a "Some checks were not successful" on github and I don't understand. It is worth noting that here I have python 3.9.2 only, and this is presumably why the tests for other versions are not being run.

My working tree is clean and up to date with origin/W0402polish, (origin being my github repo.) and git grep -i "uses of a deprecated" returns nothing.

Looking at the test output the expected output for deprecated_module_redundant needs to be updated. You can probably do this manually by changing the message in deprecated_module_redundant.txt which can be found in the tests/functional/d dir.

Current failures in pre-commit are indeed from broken main.

@omarandlorraine
Copy link
Contributor Author

Oh, thanks for taking a look.

That file didn't exist here; that's why I couldn't see it! It's new since PR#6360, which was merged in recently. So I've done the necessaries, let's see if the tests pass this time.

@omarandlorraine
Copy link
Contributor Author

Okay, now some tests for W1203 are failing. I'm certain I didn't touch W1203.

Am I missing a step here or something?

@DanielNoord
Copy link
Collaborator

@omarandlorraine I think you were just a little too quick to merge main. There is actually one more commit on main that is need to fix it. That should fix it!

@omarandlorraine
Copy link
Contributor Author

Okay, let's try this.

There has been so much toing and froing and pulling and rebasing for this simple pull request. Because someone else is working on the main branch. Is this normal?

@DanielNoord
Copy link
Collaborator

Okay, let's try this.

There has been so much toing and froing and pulling and rebasing for this simple pull request. Because someone else is working on the main branch. Is this normal?

No, this not normal. You just caught in 24h of extreme unluckiness.
First we broke one our linter on main because two connected PRs passed all checks on their own, but didn't pass when we committed them to main together.
Then, while we were in the process of fixing that linting message we broke our test suite with another set of connected PRs that passed on their own.

Both issues have now been resolved and there shouldn't be any need to merge main into this branch to get the CI to pass from now on. Just extreme unlucky... Sorry about that! 😄

@Pierre-Sassoulas
Copy link
Member

Yeah.. it's the first time main was broken since... maybe august 2021, 8 months ago (#4801), sorry for the unusually bad experience.

@omarandlorraine
Copy link
Contributor Author

Oh, that's good. I'll keep on contributing in the future then 🙂 I can see that the checks are still failing though. But I can't see the logs, maybe because I'm on my phone right now.

I'll have another look tomorrow.

@DanielNoord
Copy link
Collaborator

@omarandlorraine Just a heads up, for some reason you still seem to be missing the necessary commit. efa2a70

You might want to rebase on main. Something like git pull --rebase upstream main should work if you have our repo as your upstream.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Took the liberty to rebase because the intense activity on main made the merge strategy problematic.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2192208128

  • 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.158%

Totals Coverage Status
Change from base Build 2192205394: 0.0%
Covered Lines: 15781
Relevant Lines: 16584

💛 - Coveralls

@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas I'm seeing a lot of different emails in the commit messages. Could you give some guidance on how to add those to the contributor list?

adjust messages for W0402

updated ChangeLog

Revert "update changelogs with new checker"

Sorry, I left some crud in the changelogs. Should be fixed now.

add to doc/whatsnew/2.14.rst

Update ChangeLog

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

update functional tests for new message

hopefully also pass test on Python 3.10

change message for new test as well.
@Pierre-Sassoulas
Copy link
Member

One of the adresse was "you@example.com" so I removed it. But it's possible to create alias in https://github.com/PyCQA/pylint/blob/main/script/.contributors_aliases.json otherwise.

Thank you for your contribution @omarandlorraine !

@omarandlorraine
Copy link
Contributor Author

You're welcome!

Thanks for the help merging and closing it.

@omarandlorraine omarandlorraine deleted the W0402polish branch April 20, 2022 05:02
@omarandlorraine omarandlorraine restored the W0402polish branch April 20, 2022 05:02
@omarandlorraine omarandlorraine deleted the W0402polish branch April 20, 2022 05:02
@omarandlorraine omarandlorraine restored the W0402polish branch April 20, 2022 05:02
@omarandlorraine omarandlorraine deleted the W0402polish branch April 20, 2022 05:02
@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.14.0 milestone May 17, 2022
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.

Typo in W0402 message
5 participants