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

Add checker for empty comments #3870

Merged
merged 1 commit into from
Oct 7, 2020
Merged

Add checker for empty comments #3870

merged 1 commit into from
Oct 7, 2020

Conversation

orSolocate
Copy link
Contributor

Steps

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry.
  • If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/.
  • Write a good description on what the PR does.

Description

Added comment_ending plug-in for empty comments.

  • Empty comments in your code can be confusing in large scale programs, especially when developers are replaced frequently. If a developer who didn't write the code see an empty comment s/he can think there is a missing a comment. I know that some developers put empty comments when they want to bookmark somewhere in the code, and this is a bad practice that should be avoided.
  • This checker recognizes lines with empty comments, but also accurate enough not to raise False Positives (or else people would stop using this tool).
  • Since a message is raised only with empty comments, I could not add a test at tests/test_functional.py folder, so I wrote a unittest instead.

Type of Changes

IsDone Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

None

@coveralls
Copy link

coveralls commented Oct 2, 2020

Coverage Status

Coverage increased (+0.01%) to 90.723% when pulling d66cd83 on orSolocate:orb_branch into 98d2c06 on PyCQA:master.

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.

Thank you for the check, and yes evidently we can't do functional tests for this one.

I made some suggestion that might not be in the spirit of the original checker. I think there is also empty comments created voluntarily to separate code part. I think it's actually better to create function/class/file/package, if you feel the need to separate different part of the code with empty comment. Your checker seems to be a way to check for this almost similar use case at the same time. Let me know what you think.

pylint/extensions/comment_ending.py Outdated Show resolved Hide resolved
pylint/extensions/comment_ending.py Outdated Show resolved Hide resolved
tests/extensions/data/comment_ending.py Outdated Show resolved Hide resolved
tests/extensions/test_comment_ending.py Outdated Show resolved Hide resolved
pylint/extensions/comment_ending.py Outdated Show resolved Hide resolved
doc/whatsnew/2.7.rst Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas changed the title Added comment-ending plug-in for empty comments Add checker for empty comments Oct 5, 2020
@Pierre-Sassoulas
Copy link
Member

Thank for the new checker :) Could you rebase on the latest master, please ?

@orSolocate
Copy link
Contributor Author

@Pierre-Sassoulas I fetched the remote master and merged my branch into it. then uploaded my branch here. I wasn't sure if that is what you wanted me to do, or you wanted me to push the Master also?

@Pierre-Sassoulas
Copy link
Member

Thank you, could you rebase on it instead of merging, please ?

@orSolocate
Copy link
Contributor Author

@Pierre-Sassoulas I think it is fixed now.

PR Review:

refactor checker messages, test cases added: empty commit case, comments row case. unittest - pathlib replaces os.path.

python3.5 fix for pathlib library use

refactor `comment_ending` to `empty_comment`

empty-comment test fixed (after Rebase)
@orSolocate
Copy link
Contributor Author

I pushed again to get rid of an extra commit. There is only 1 commit for my checker now with all the fixes :)

@Pierre-Sassoulas Pierre-Sassoulas merged commit 83bc759 into pylint-dev:master Oct 7, 2020
@Pierre-Sassoulas
Copy link
Member

Thank you for the rebase and congratulation on the new checker 👍

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.

4 participants