Smalltalk › Pharo › Pharo Smalltalk Developers

about quality assistant

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

23 messages Options

stepharo

about quality assistant

HI yuriy

it would be good that quality assistant uses the same vocabulary than
codecritics.
for example
skip -> ban?
marks as false positif?

Stef

Uko2

Re: about quality assistant

HI,

yes, ban may make sense. I think that “false positive” is a bit incorrect. For example, consider the “probably missing yourself” rule. Even if you are not missing it, the rule is not wrong. It just warns you that you should be carful with this case, because it is easy to screw up something. Now even while this is not a false positive, you want to skip the rule for this method.

That was the philosophy behind the name.

Uko

> On 23 Jul 2015, at 22:29, stepharo <[hidden email]> wrote:
>
> HI yuriy
>
> it would be good that quality assistant uses the same vocabulary than codecritics.
> for example
> skip -> ban?
> marks as false positif?
>
> Stef
>

stepharo

Re: about quality assistant

Le 23/7/15 23:49, Yuriy Tymchuk a écrit :
> HI,
>
> yes, ban may make sense. I think that “false positive” is a bit incorrect. For example, consider the “probably missing yourself” rule. Even if you are not missing it, the rule is not wrong. It just warns you that you should be carful with this case, because it is easy to screw up something. Now even while this is not a false positive, you want to skip the rule for this method.
>
> That was the philosophy behind the name.
I do not know. We discussed a lot in the past about this vocabulary and
it would be good to have one. I do not understand the diff between this
is useless and skip.
I prefer
ban this rule = do not run it
false positive = rule is ok but wrong here

Stef

>
> Uko
>
>> On 23 Jul 2015, at 22:29, stepharo <[hidden email]> wrote:
>>
>> HI yuriy
>>
>> it would be good that quality assistant uses the same vocabulary than codecritics.
>> for example
>> skip -> ban?
>> marks as false positif?
>>
>> Stef
>>
>
>

Stephan Eggermont-3

Re: about quality assistant

When I say 'this is useless' I might be interested to
explain why I think so.

Stephan

Uko2

Re: about quality assistant

In reply to this post by stepharo

On 24 Jul 2015, at 19:17, stepharo <[hidden email]> wrote:

Le 23/7/15 23:49, Yuriy Tymchuk a écrit :
HI,

yes, ban may make sense. I think that “false positive” is a bit incorrect. For example, consider the “probably missing yourself” rule. Even if you are not missing it, the rule is not wrong. It just warns you that you should be carful with this case, because it is easy to screw up something. Now even while this is not a false positive, you want to skip the rule for this method.

That was the philosophy behind the name.
I do not know. We discussed a lot in the past about this vocabulary and it would be good to have one. I do not understand the diff between this is useless and skip.
I prefer
ban this rule = do not run it
false positive = rule is ok but wrong here

Do you mean ban rule completely? On which level? For a developer (machine)?

Then again, skip works as former “false positive”. But in my opinion “false positive” is not a good name from the logical point of view. You have a rule that detects if you have no #yourself message in the end of a cascade. Now do people really mark this as “false positive” if the rule detects cascades that have #yourself message in the end? Or they do it if they don’t want to see the critic? Then it’s not false positive, then they just want to hide or “skip” the rule. This is fun part from statistics to name things as false positives. But if we program a rule in such way, that it is fuzzy, I think that it’s not correct to call it as false positive. Maybe it’s critics are useless more times that they are helpful, and if the critic is correct, the issue is not that important. Then probably we should just remove the rule or rewrite it.

Now with useless button, the only reason is to collect data on which rules are actually useless to people [1], so we can take a look at them in a first place. It doest not affect the functionality.

Uko

[1]: http://renraku.inf.usi.ch/rules

Stef

Uko

On 23 Jul 2015, at 22:29, stepharo <[hidden email]> wrote:

HI yuriy

it would be good that quality assistant uses the same vocabulary than codecritics.
for example
skip -> ban?
marks as false positif?

Stef

Uko2

Re: about quality assistant

In reply to this post by Stephan Eggermont-3

> On 24 Jul 2015, at 19:31, Stephan Eggermont <[hidden email]> wrote:
>
> When I say 'this is useless' I might be interested to
> explain why I think so.

Hi, yes, this is planned, and is the main goal, because I need to understand what is bad. So it’s in my TODO list, and at some point I will at it. Thanks for interest.

Uko

>
> Stephan
>
>
>

stepharo

Re: about quality assistant

In reply to this post by Uko2

I prefer
ban this rule = do not run it
false positive = rule is ok but wrong here

Do you mean ban rule completely? On which level? For a developer (machine)?

in the codecritcis the scope is clear.
On a class or a package.

Then again, skip works as former “false positive”. But in my opinion “false positive” is not a good name from the logical point of view. You have a rule that detects if you have no #yourself message in the end of a cascade. Now do people really mark this as “false positive” if the rule detects cascades that have #yourself message in the end? Or they do it if they don’t want to see the critic? Then it’s not false positive, then they just want to hide or “skip” the rule.

Sorry but I do not understand your argument.
For me this is simple:
the rule tells me something is wrong and it is not.
-> so I mark it as false positive.

This is fun part from statistics to name things as false positives. But if we program a rule in such way, that it is fuzzy, I think that it’s not correct to call it as false positive. Maybe it’s critics are useless more times that they are helpful, and if the critic is correct, the issue is not that important. Then probably we should just remove the rule or rewrite it.

Now with useless button, the only reason is to collect data on which rules are actually useless to people [1], so we can take a look at them in a first place. It doest not affect the functionality.

I do not like useless. Because it is too violent and unclear.
Please again have a look at the vocabulary used in the CodeCritics and the chapter that describes it.
Or you should fix everything but we should not have two different vocabularies.
There is enough confusing terms in Pharo so that we do not add more.

Stef

Uko

[1]: http://renraku.inf.usi.ch/rules

Stef

Uko

On 23 Jul 2015, at 22:29, stepharo <[hidden email]> wrote:

HI yuriy

it would be good that quality assistant uses the same vocabulary than codecritics.
for example
skip -> ban?
marks as false positif?

Stef

abergel

Re: about quality assistant

> I do not like useless. Because it is too violent and unclear.

+1 on this.

For example, in the class RTRoassalExample, there are many methods. Too much for the quality assistant. Finally, I did not press ‘useless’ because I still think this is a useful rule, but not for that class.

Cheers,
Alexandre
--
_,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:
Alexandre Bergel http://www.bergel.eu
^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.

stepharo

Re: about quality assistant

I think that the impact of an action should be clear.
From a user perspective I should not guess not even ask yourself a
question.

Alex with codrCritcis you can mark that a ruel should not apply to a
class (ban the rule on the class).
Stef

Le 26/7/15 14:46, Alexandre Bergel a écrit :
>> I do not like useless. Because it is too violent and unclear.
> +1 on this.
>
> For example, in the class RTRoassalExample, there are many methods. Too much for the quality assistant. Finally, I did not press ‘useless’ because I still think this is a useful rule, but not for that class.
>
> Cheers,
> Alexandre

abergel

Re: about quality assistant

This is exactly what I want

Alexandre

> Le 26 juil. 2015 à 17:07, stepharo <[hidden email]> a écrit :
>
> Alex with codrCritcis you can mark that a ruel should not apply to a class (ban the rule on the class).

Uko2

Re: about quality assistant

In reply to this post by stepharo

On 26 Jul 2015, at 10:55, stepharo <[hidden email]> wrote:

I prefer
ban this rule = do not run it
false positive = rule is ok but wrong here

Do you mean ban rule completely? On which level? For a developer (machine)?

in the codecritcis the scope is clear.
On a class or a package.

How is it different from false positive on a method or a class. Please do not misunderstand me, but this is a part of story telling. If you don’t want to see a critic (of a class or a method), you mark it as false positive. Now you can also ban rule on the level of a class or package, so you will not see the critics about it. For me this two stories are intersecting. Why not simply say: “If you don’t want to see this critic anymore (for any reason), then you can ban it on the level of method, class, tag, package, or whole system”. This way the story is simpler.

Then again, skip works as former “false positive”. But in my opinion “false positive” is not a good name from the logical point of view. You have a rule that detects if you have no #yourself message in the end of a cascade. Now do people really mark this as “false positive” if the rule detects cascades that have #yourself message in the end? Or they do it if they don’t want to see the critic? Then it’s not false positive, then they just want to hide or “skip” the rule.

Sorry but I do not understand your argument.
For me this is simple:
the rule tells me something is wrong and it is not.
-> so I mark it as false positive.

This is fun part from statistics to name things as false positives. But if we program a rule in such way, that it is fuzzy, I think that it’s not correct to call it as false positive. Maybe it’s critics are useless more times that they are helpful, and if the critic is correct, the issue is not that important. Then probably we should just remove the rule or rewrite it.

Now with useless button, the only reason is to collect data on which rules are actually useless to people [1], so we can take a look at them in a first place. It doest not affect the functionality.

I do not like useless. Because it is too violent and unclear.
Please again have a look at the vocabulary used in the CodeCritics and the chapter that describes it.
Or you should fix everything but we should not have two different vocabularies.
There is enough confusing terms in Pharo so that we do not add more.

I understand. I will re-read the vocabulary and fix discrepancies. But please read my argument bout duplicated behavior of banking and marking as false positive, I think that it may be confusing.

Also “Useless" button does not do anything besides sending me an information that this critic was useless to somebody. Ideally there should be no button like this and I should be able to collect user actions that they perform. But from server/privacy/time perspective it was easier for me to make a simple button. Would be nice to have an advanced data collection, but I am not sure that I will have time now to program all that, so if it is bothering so much, I can simply remove it.

Uko

Stef

Uko

[1]: http://renraku.inf.usi.ch/rules

Stef

Uko

On 23 Jul 2015, at 22:29, stepharo <[hidden email]> wrote:

HI yuriy

it would be good that quality assistant uses the same vocabulary than codecritics.
for example
skip -> ban?
marks as false positif?

Stef

Uko2

Re: about quality assistant

In reply to this post by abergel

> On 26 Jul 2015, at 14:46, Alexandre Bergel <[hidden email]> wrote:
>
>> I do not like useless. Because it is too violent and unclear.
>
> +1 on this.
>
> For example, in the class RTRoassalExample, there are many methods. Too much for the quality assistant. Finally, I did not press ‘useless’ because I still think this is a useful rule, but not for that class.

This is sad, and shows that I completely failed with that button. Because ideally you had to press it so I can know that this rule had a useful case for somebody.

Uko

>
> Cheers,
> Alexandre
> --
> _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:
> Alexandre Bergel http://www.bergel.eu
> ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
>
>
>
>

Peter Uhnak

Re: about quality assistant

Would be nice to have an advanced data collection, but I am not sure that I will have time now to program all that

There is already data collection for spotter and there was chatter about making the collection tool more generic, but I don't think any progress is on that.

So what exactly is "Useless" for? If there was some input box where I could describe why I think its useless in that particular context or whatever...

However I have question about two more cases, not sure if even possible to implement

case 1:

~~~~~~~~~~~~~~~~~~~~~~~~

beFinal

self isFinal: true

~~~~~~~~~~~~~~~~~~~~~~~~

"Unary "accessing" method without explicit return"

#beSomething is quite common pattern (even recommended in Kent's Best Practice Patterns), maybe it should check for name 'be'<Uppercase> before applying the critic?

case 2:

in Roassal it's very common to create a shape but return an element

~~~~~~~~~~~~~~~~~~~~~~~~

RTEllipse new

size: 14;

color: Color black;

element

~~~~~~~~~~~~~~~~~~~~~~~~

RTBox new

elementOn: aModel

~~~~~~~~~~~~~~~~~~~~~~~~

but of course it complains about missing ";yourself".

However I do not really want to ban the critic, because there may other legitimate applications in the method/class; and in Roassal-heavy code it's a nuisance.

So it would require context-aware checks (probably user-defined?) like "if receiver is subclass of RTShape then don't apply this (yourself) rule".

Cheers,

Peter

Uko2

Re: about quality assistant

In reply to this post by stepharo

On 26 Jul 2015, at 10:55, stepharo <[hidden email]> wrote:

I prefer
   ban this rule = do not run it
   false positive = rule is ok but wrong here

Do you mean ban rule completely? On which level? For a developer (machine)?

in the codecritcis the scope is clear.
On a class or a package.

Then again, skip works as former “false positive”. But in my opinion “false positive” is not a good name from the logical point of view. You have a rule that detects if you have no #yourself message in the end of a cascade. Now do people really mark this as “false positive” if the rule detects cascades that have #yourself message in the end? Or they do it if they don’t want to see the critic? Then it’s not false positive, then they just want to hide or “skip” the rule.

Sorry but I do not understand your argument.
For me this is simple:
    the rule tells me something is wrong and it is not.
    -> so I mark it as false positive.

One more use case: at the moment most of “useless” reports has the RBClassNotReferencedRule. If you have a class that is not referenced and it is reported by the rule, it is definitely not a false positive. But never the less, maybe you want to ban it, because the class is a configuration (or something similar). So maybe it makes sense to leave only a single “ban” option, and then allow user to provide a detail about the ban (whether it was a false positive, or user simply does not care).

Yuriy

This is fun part from statistics to name things as false positives. But if we program a rule in such way, that it is fuzzy, I think that it’s not correct to call it as false positive. Maybe it’s critics are useless more times that they are helpful, and if the critic is correct, the issue is not that important. Then probably we should just remove the rule or rewrite it.

Now with useless button, the only reason is to collect data on which rules are actually useless to people [1], so we can take a look at them in a first place. It doest not affect the functionality.

I do not like useless. Because it is too violent and unclear.
Please again have a look at the vocabulary used in the CodeCritics and the chapter that describes it.
Or you should fix everything but we should not have two different vocabularies.
There is enough confusing terms in Pharo so that we do not add more.

Stef

Uko

[1]: http://renraku.inf.usi.ch/rules

Stef

Uko

On 23 Jul 2015, at 22:29, stepharo <[hidden email]> wrote:

HI yuriy

it would be good that quality assistant uses the same vocabulary than codecritics.
for example
skip -> ban?
marks as false positif?

Stef

Uko2

Re: about quality assistant

In reply to this post by Peter Uhnak

On 27 Jul 2015, at 18:11, Peter Uhnák <[hidden email]> wrote:

Would be nice to have an advanced data collection, but I am not sure that I will have time now to program all that

There is already data collection for spotter and there was chatter about making the collection tool more generic, but I don't think any progress is on that.

So what exactly is "Useless" for? If there was some input box where I could describe why I think its useless in that particular context or whatever…

Yes, there should be an input box, it will be aded there as soon as I’m done with other issues and the button remains.

However I have question about two more cases, not sure if even possible to implement

case 1:
~~~~~~~~~~~~~~~~~~~~~~~~
beFinal
self isFinal: true
~~~~~~~~~~~~~~~~~~~~~~~~
"Unary "accessing" method without explicit return"

#beSomething is quite common pattern (even recommended in Kent's Best Practice Patterns), maybe it should check for name 'be'<Uppercase> before applying the critic?

This is a good proposal. Would be nice to hear other opinions. Maybe we can open an issue on the bug tracker so people can comment.

case 2:
in Roassal it's very common to create a shape but return an element
~~~~~~~~~~~~~~~~~~~~~~~~
RTEllipse new
size: 14;
color: Color black;
element
~~~~~~~~~~~~~~~~~~~~~~~~
or
~~~~~~~~~~~~~~~~~~~~~~~~
RTBox new
elementOn: aModel
~~~~~~~~~~~~~~~~~~~~~~~~

but of course it complains about missing ";yourself".

However I do not really want to ban the critic, because there may other legitimate applications in the method/class; and in Roassal-heavy code it's a nuisance.
So it would require context-aware checks (probably user-defined?) like "if receiver is subclass of RTShape then don't apply this (yourself) rule”.

Yes, it’s a very good point. It is in my plans to see how we can allow frameworks to provide their own critics and interact/override the default ones. For now all I can suggest is to ban the critic on the level of the class, or if we will reach the common sense there can be a ban on the level of package tag, so you can keep all the roassal-related classes in one tag and ban the rule for them.

Uko

Cheers,
Peter

stepharo

Re: about quality assistant

In reply to this post by Uko2

How is it different from false positive on a method or a class. Please do not misunderstand me, but this is a part of story telling.

This is the same.
We should be able to say: this rule is in general correct but for this specific case (method, class, package) it is not correct (false positive)

If you don’t want to see a critic (of a class or a method), you mark it as false positive.

I do not understand "don't want to see a critic"
I do not want to get reminded that I should take action to address a quality problem when I know that there is none.

Now you can also ban rule on the level of a class or package, so you will not see the critics about it.

Exact. There is an overlap between ban a rule at package level and marking it as a false positive.
Banning is do not check
false positive is I run it and I tell you that it is wrong.
So people have to decide what they want.

For me this two stories are intersecting. Why not simply say: “If you don’t want to see this critic anymore (for any reason), then you can ban it on the level of method, class, tag, package, or whole system”. This way the story is simpler.

We could but we lose a granularity.
When I ban a rule I do not want to run it.
While a false positive could be reevaluated if I change me code
we could have a button that say rerun false positives.

Then again, skip works as former “false positive”. But in my opinion “false positive” is not a good name from the logical point of view. You have a rule that detects if you have no #yourself message in the end of a cascade. Now do people really mark this as “false positive” if the rule detects cascades that have #yourself message in the end? Or they do it if they don’t want to see the critic? Then it’s not false positive, then they just want to hide or “skip” the rule.

Sorry but I do not understand your argument.
For me this is simple:
the rule tells me something is wrong and it is not.
-> so I mark it as false positive.

This is fun part from statistics to name things as false positives. But if we program a rule in such way, that it is fuzzy, I think that it’s not correct to call it as false positive. Maybe it’s critics are useless more times that they are helpful, and if the critic is correct, the issue is not that important. Then probably we should just remove the rule or rewrite it.

Now with useless button, the only reason is to collect data on which rules are actually useless to people [1], so we can take a look at them in a first place. It doest not affect the functionality.

I do not like useless. Because it is too violent and unclear.
Please again have a look at the vocabulary used in the CodeCritics and the chapter that describes it.
Or you should fix everything but we should not have two different vocabularies.
There is enough confusing terms in Pharo so that we do not add more.

I understand. I will re-read the vocabulary and fix discrepancies. But please read my argument bout duplicated behavior of banking and marking as false positive, I think that it may be confusing.

I'm not against your arguments I do not understand them.

Also “Useless" button does not do anything besides sending me an information that this critic was useless to somebody. Ideally there should be no button like this and I should be able to collect user actions that they perform.

So far I do not understand skip and useless difference. Now I get it. There are two different action. One is about feedback and the other is banning/

But from server/privacy/time perspective it was easier for me to make a simple button. Would be nice to have an advanced data collection, but I am not sure that I will have time now to program all that, so if it is bothering so much, I can simply remove it.

In fact by crawling the metadata of package we can see if a rule is useless: people will ban it.

Uko

Stef

Uko

[1]: http://renraku.inf.usi.ch/rules

Stef

Uko

On 23 Jul 2015, at 22:29, stepharo <[hidden email]> wrote:

HI yuriy

it would be good that quality assistant uses the same vocabulary than codecritics.
for example
skip -> ban?
marks as false positif?

Stef

stepharo

Re: about quality assistant

In reply to this post by Uko2

I do not like useless. Because it is too violent and unclear.

>> +1 on this.
>>
>> For example, in the class RTRoassalExample, there are many methods. Too much for the quality assistant. Finally, I did not press ‘useless’ because I still think this is a useful rule, but not for that class.
> This is sad, and shows that I completely failed with that button.
Yes
This is not clear that this is a button about feedback

> Because ideally you had to press it so I can know that this rule had a useful case for somebody
>
> Uko
>
>> Cheers,
>> Alexandre
>> --
>> _,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:
>> Alexandre Bergel http://www.bergel.eu
>> ^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;._,.;:~^~:;.
>>
>>
>>
>>
>
>

stepharo

Re: about quality assistant

In reply to this post by Uko2

Then again, skip works as former “false positive”. But in my opinion “false positive” is not a good name from the logical point of view. You have a rule that detects if you have no #yourself message in the end of a cascade. Now do people really mark this as “false positive” if the rule detects cascades that have #yourself message in the end? Or they do it if they don’t want to see the critic? Then it’s not false positive, then they just want to hide or “skip” the rule.

Sorry but I do not understand your argument.
For me this is simple:
the rule tells me something is wrong and it is not.
-> so I mark it as false positive.

One more use case: at the moment most of “useless” reports has the RBClassNotReferencedRule. If you have a class that is not referenced and it is reported by the rule, it is definitely not a false positive. But never the less, maybe you want to ban it, because the class is a configuration (or something similar). So maybe it makes sense to leave only a single “ban” option, and then allow user to provide a detail about the ban (whether it was a false positive, or user simply does not care).

I see
Ban = do not report it again. Do not show it as a problem.
You can call it: "not a problem"

Yuriy

This is fun part from statistics to name things as false positives. But if we program a rule in such way, that it is fuzzy, I think that it’s not correct to call it as false positive. Maybe it’s critics are useless more times that they are helpful, and if the critic is correct, the issue is not that important. Then probably we should just remove the rule or rewrite it.

Now with useless button, the only reason is to collect data on which rules are actually useless to people [1], so we can take a look at them in a first place. It doest not affect the functionality.

I do not like useless. Because it is too violent and unclear.
Please again have a look at the vocabulary used in the CodeCritics and the chapter that describes it.
Or you should fix everything but we should not have two different vocabularies.
There is enough confusing terms in Pharo so that we do not add more.

Stef

Uko

[1]: http://renraku.inf.usi.ch/rules

Stef

Uko

On 23 Jul 2015, at 22:29, stepharo <[hidden email]> wrote:

HI yuriy

it would be good that quality assistant uses the same vocabulary than codecritics.
for example
skip -> ban?
marks as false positif?

Stef

abergel

Re: about quality assistant

In reply to this post by Uko2

But, what does the useless button do? Does it remove from the rules quality assistant uses forever? Or is it per class? the rule "Too many methods" is useless for the class RTexample, but it is useful for other classes

Alexandre

> Le 27 juil. 2015 à 12:53, Yuriy Tymchuk <[hidden email]> a écrit :
>
> This is sad, and shows that I completely failed with that button. Because ideally you had to press it so I can know that this rule had a useful case for somebody.

stepharo

Re: about quality assistant

Le 28/7/15 11:43, Alexandre Bergel a écrit :
> But, what does the useless button do? Does it remove from the rules quality assistant uses forever? Or is it per class? the rule "Too many methods" is useless for the class RTexample, but it is useful for other classes

Alex
I was confused too :)
I guess that it does nothing. Just report to the server of yuriy that
you think that this rule is useless.
So there is a clear problem at the UI level. The user cannot
disntinguish what he is doing:
marking false positive
or giving feedback in general.

Stef
>
> Alexandre
>
>
>
>> Le 27 juil. 2015 à 12:53, Yuriy Tymchuk <[hidden email]> a écrit :
>>
>> This is sad, and shows that I completely failed with that button. Because ideally you had to press it so I can know that this rule had a useful case for somebody.
>