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

NoUselessCommentFixer - Introduction #4078

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@kubawerlos
Contributor

kubawerlos commented Nov 1, 2018

Fixer to remove useless comments, few question:

  • should it be extended for interfaces and/or traits?
  • should it be extended for properties and/or functions?

Ping @dkarlovi

@julienfalque

This comment has been minimized.

Member

julienfalque commented Nov 2, 2018

  • should it be extended for interfaces and/or traits?
  • should it be extended for properties and/or functions?

Definitely 👍 for me, but then the fixer should have a more generic name.

@julienfalque julienfalque added the feature label Nov 2, 2018

@kubawerlos kubawerlos changed the title from Add NoUselessClassCommentFixer to Add NoUselessCommentFixer Nov 2, 2018

@dkarlovi

This comment has been minimized.

dkarlovi commented Nov 12, 2018

I've just tested this with my codebase, it works great. See dkarlovi/xezilaires#52 for outcome (with other settings too).

if ($tokens[$nextIndex]->isGivenKind([T_CLASS, T_INTERFACE, T_TRAIT])) {
$newContent = Preg::replace(
'/((^|\R)\h*(#|\/*\**))\h*\b(Class|Interface|Trait)\h+[A-Za-z0-1\\\\_]+\.?(\h*\R\h*\*?|\h*$)/i',

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Nov 12, 2018

Contributor
<?php
/**
 * Class Test
 */
trait Test {}

Will this get removed? (I think so)
Should it? (I think not)

This comment has been minimized.

@kubawerlos

kubawerlos Nov 13, 2018

Contributor

This is not only useless, but might be misleading - e.g. if there is documentation generated from comments.

Show resolved Hide resolved src/Fixer/Comment/NoUselessCommentFixer.php Outdated
);
} elseif ($tokens[$nextIndex]->isGivenKind(T_FUNCTION)) {
$newContent = Preg::replace(
'/((^|\R)\h*(#|\/*\**))\h*\b((Gets?|SetS?)\h+[A-Za-z0-1\\\\_]+|[A-Za-z0-1\\\\_]+\h+constructor)\.?(\h*\R\h*\*?|\h*$)/i',

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Nov 12, 2018

Contributor
<?php
/**
 * Gets A
 */
function setA($a) {}

Will this get removed? (I think so)
Should it? (I think not)

This comment has been minimized.

@kubawerlos

kubawerlos Nov 13, 2018

Contributor

Should it? (I think not)

Why not? It is useless.

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Nov 13, 2018

Contributor

That's the question though, it really depends on what is stated. I'd say if it's not the same, don't remove it.

This comment has been minimized.

@kubawerlos

kubawerlos Nov 13, 2018

Contributor

I assumed that "set"/"get" + 1 word is useless - I cannot imagine example when it could be helpful. I'd like the "Gets A" to be removed if someone tries to add it for "setA" function.

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Nov 13, 2018

Contributor

I'll leave the final call to the maintainers.

I'd like to enable this fixer in my projects as well, however the current state makes it too risky for me 😞

edit: however if it stays the same, please add some additional examples and tests, as this behaviour is currently undocumented

Show resolved Hide resolved src/Fixer/Comment/NoUselessCommentFixer.php Outdated
@dkarlovi

This comment has been minimized.

dkarlovi commented Nov 12, 2018

BTW, one thing I noticed is, if I have

class Foo
{
    /*
     * @return Foo
     */
    public function foo(): self
    {
        return new self;
    }
}

It will not strip that (but will with @return self).

);
} elseif ($tokens[$nextIndex]->isGivenKind(T_FUNCTION)) {
$newContent = Preg::replace(
'/((^|\R)\h*(#|\/*\**))\h*\b((Gets?|SetS?)\h+[A-Za-z0-1\\\\_]+|[A-Za-z0-1\\\\_]+\h+constructor)\.?(\h*\R\h*\*?|\h*$)/i',

This comment has been minimized.

@dmvdbrugge

dmvdbrugge Nov 12, 2018

Contributor
<?php
/**
 * Set Thing
 */
function setSomethingElse($obj) {}

Looking at the code, this too will get removed. I think again that it should not.

This comment has been minimized.

@kubawerlos

kubawerlos Nov 13, 2018

Contributor

Why? Seems useless - when the comment is useful it contains more than one word after "set"/"get".

@dmvdbrugge

This comment has been minimized.

Contributor

dmvdbrugge commented Nov 12, 2018

BTW, one thing I noticed (...)
It will not strip that (but will with @return self).

That's no_superfluous_phpdoc_tags and has nothing to do with the fixer under PR. You could create a new issue if you think it is a bug.

@dkarlovi

This comment has been minimized.

dkarlovi commented Nov 12, 2018

@dmvdbrugge sorry, you're right. 👍

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Nov 12, 2018

@kubawerlos

This comment has been minimized.

Contributor

kubawerlos commented Nov 13, 2018

@ostrolucky can you give an example (in a form of test case)?

@kubawerlos kubawerlos force-pushed the kubawerlos:feature/no-useless-class-comment branch from b5d3708 to e1f381d Nov 13, 2018

@julienfalque

This comment has been minimized.

Member

julienfalque commented Nov 13, 2018

Looking at the discussion, maybe the "comment uselessness detection" part should be configurable somehow, with sensitive defaults?

@kubawerlos

This comment has been minimized.

Contributor

kubawerlos commented Nov 13, 2018

Any suggestions? Like an option level with values low and high?

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Nov 13, 2018

image

generates

<?php
/**
 * Created by IntelliJ IDEA.
 * User: g9735
 * Date: 13/11/18
 * Time: 14:19
 */

class Test
{

}

yes, same for phpstorm, so take both in account

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Nov 13, 2018

I like flexibility of SlevomatCodingStandard\Sniffs\Commenting\ForbiddenCommentsSniff like demonstrated. User can supply any regexes it wants. Can Fixer accept dynamic option values as well?

@julienfalque

This comment has been minimized.

Member

julienfalque commented Nov 13, 2018

Yeah I would rather go with regexps than levels, they provide much more flexibility. But I wonder if that would work very well. I mean, maybe one wants to remove comments like "returns foo" from method getFoo() but not from other methods. Not sure how to allow this kind of flexibility though.

@kubawerlos

This comment has been minimized.

Contributor

kubawerlos commented Nov 13, 2018

An array with "item that follows" (like class or function) => regex? Something like

[
    '.*Interface$' => '^Interface [A-Za-z0-9]+$',
    '^get[A-Za-z0-9]+$' => '^gets? [A-Za-z0-9]+',
]

But then for every "item" matching any key regex we would have run value regex against it's comment every line.

@dmvdbrugge

This comment has been minimized.

Contributor

dmvdbrugge commented Nov 15, 2018

Are there other rules that do regexes instead of levels already? Because I think it would introduce too much flexibility.

Personally, I'd be fine with just options strict or all, where strict would be just the same as all examples now suggest (with optional 's' and period), and all would be the current implementation.

@SpacePossum SpacePossum changed the title from Add NoUselessCommentFixer to NoUselessCommentFixer - Introduction Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment