String concatenation critique is too strict

[macta]

The code critic shows a warning when you concatenate 2 strings and suggests using a Stream - I think that showing this for a simple ^'hello', 'world' is not true - and it should report this error only over 2 or possibly 3 concatenations?

[macta]

I think the fix is something like the following (hack)

RBStringConcatenationRule>>#performsConcatenation: aNode
    | i |

    aNode isBlock ifFalse: [ ^ false ].
    i := 0.
    aNode nodesDo: [ :node |
        (node isMessage and: [ 
        node selector = #, ]) ifTrue: [ 
            i := i + 1.
            i > 1 ifTrue: [ ^true ] ] ].

    ^ false

But this needs more investigation as I don't know if it breaks other things that rely on this behaviour

[astares]

I would disagree. From my perspective it is fine as it sometimes really reveals problems in some code that should use a stream even for a 2 string concatenation.

If it is too strict for you can customize already and ban the rule on method, class or package level.

[bencoman]

On Tue, 12 Mar 2019 at 15:50, Astares notifications@github.com wrote:

I would disagree. From my perspective it is fine as it sometimes really reveals problems in some code that should use a stream even for a 2 string concatenation.

I've observed disparate views in the community about the validity of "Stream versus concatenation" philosophy. What is important to you that this rule reveals?

If it is too strict for you can customize already and ban the rule on method, class or package level.

cheers -ben

[macta]

I actually don’t want to ban the rule completely, so maybe this should be a way to specify the concatenation threshold to X for class, package etc. As a starting point, a simple classic vs advanced setting (the latter allowing X or maybe set to 2 or 3 would be a good starting point).

But I’m also interested in where a 2 string concatenation could be another bug? I certainly don’t think it’s more efficient to have a stream in that case (but haven’t seen it benched in a while - so probably should redo that test)

[stale[bot]]

To limit bug bankruptcy (see https://www.joelonsoftware.com/2012/07/09/software-inventory/) this issue has been automatically marked as stale because it has not had any activity in 6 months. It will be closed in 1 month if no further activity occurs. If this issue remains important to you, please comment to reactivate the issue. Thank you for your contributions.

Joel on Software

Software Inventory

Imagine, for a moment, that you came upon a bread factory for the first time. At first it just looks like a jumble of incomprehensible machinery with a few people buzzing around. As your eyes adjus…

[guillep]

Hi all, this seems like a controversial issue. I remember a discussion in the mailing list not so long about this.

Can we do a follow up?

[svenvc]

A single concatenation is always more efficient than streaming, two probably also. In most cases, it will also be more readable and intention revealing.

In printOn: style methods, where you write to a stream, concatenations and asString conversions are always a bad idea (mostly due to the garbage they generate).

In little used code, like error handling, there should be less restrictions.

Expressing all this as a rule will be a challenge.

[guillep]

A single concatenation is always more efficient than streaming, two probably also. In most cases, it will also be more readable and intention revealing.

In printOn: style methods, where you write to a stream, concatenations and asString conversions are always a bad idea (mostly due to the garbage they generate).

In little used code, like error handling, there should be less restrictions.

Expressing all this as a rule will be a challenge.

This makes me think that the rule is not of much value. To make it have some value we should encode super complicated assumptions, that will potentially be fragile.

And even with that, is that worth it? Thoughts? I'd like to have the ability to make polls here xD

[macta]

I don’t think its so complicated in the worst case scenario - if you do more than 3 concatenations (e.g. 4 +) give the error it gives now. For the cases of 2 and 3 - that’s where is depends - so for now bump up the number and we can revisit? At the moment it complains too much for it to be viable.

On 6 Apr 2020, at 16:32, Guille Polito notifications@github.com wrote:

A single concatenation is always more efficient than streaming, two probably also. In most cases, it will also be more readable and intention revealing.

In printOn: style methods, where you write to a stream, concatenations and asString conversions are always a bad idea (mostly due to the garbage they generate).

In little used code, like error handling, there should be less restrictions.

Expressing all this as a rule will be a challenge.

This makes me think that the rule is not of much value. To make it have some value we should encode super complicated assumptions, that will potentially be fragile.

And even with that, is that worth it? Thoughts? I'd like to have the ability to make polls here xD

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pharo-project/pharo/issues/2794#issuecomment-609866933, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKL3VYYQL3ZG3SRK3R7LITRLHYZBANCNFSM4G5EL4MA.

[no-response[bot]]

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.

[macta]

Is the bot correctly closing things? I did reply - and it looks like @guillep tagged it (but don't understand the more-information-needed tag? Its simply noise the way it is, so bump up the number or remove the check completely? A more sophisticated analyses over 3 concatenations - e.g. you are doing this in a loop and so should genuinely use a stream feel like a V2 improvement?

[guillep]

ah, sorry, I did not know there was a bot that reacted to the "more information needed" tag. I don't think these bots work for us... There are too many and the community is clearly not engaged with them, creating extra traffic and noise...

[MarcusDenker]

I have removed the stale bot config file. So it should be disabled now

[kasperosterbye]

TheprintString insideprintOn: has a rule warning against bad practice.

Personally, I just ask the "do not use #," rule to shut up if I deem it wrong in my specific case. Besides, that rule has a rewrite connected with it, so it is easy to fix.

I suggest to close it now.

[macta]

Why not make the number 4 and be done with it? This way it would catch obvious bad practice and one day we may be able to encode something in the gray area?

[kasperosterbye]

I agree with you at the logical level @macta. However, I tried to look at the code for the rule. While it is not immediately clear for me how to check for a,b,c,d, even (a,b),(c,d) becomes really subtle, not to mention ... do: [:x| s := s, x] or ... inject: '' into: [:s :x | s , x] etc. What is "4" in those situations.

I therefore still suggest to close it now. If you just meant catching a,b,c,d I believe it is doable.

I must admit that I just bring up this menu (by clicking on the X next to the info below the code):

[macta]

Is there a misunderstaning here? I’m nat saying try to check for "a,b,c,d, even (a,b),(c,d)” - thats where the difficulty lies (which I think we all agree on) - I’m simply saying that at the moment it identifies a basic number of concatenations - and it fails at 2 right now - can’t we just set that number to 4 or 5 (e.g. increase the threshold)? This would preserve the intent of the check to look at really bad examples - but may cause it to miss others it should catch (but its too hard) - but I think that would be better than simply defaulting this check to off.

If this is all too complicated - then rather than close this - we should remove the lint check completely - as at the moment it just creates too much noise as a default.

On Mon, 8 Feb 2021, at 6:11 AM, Kasper Østerbye wrote:

I agree with you at the logical level @macta https://github.com/macta. However, I tried to look at the code for the rule. While it is not immediately clear for me how to check for a,b,c,d, even (a,b),(c,d) becomes really subtle, not to mention ... do: [:x| s := s, x] or ... inject: '' into: [:s :x | s , x] etc. What is "4" in those situations.

I therefore still suggest to close it now. If you just meant catching a,b,c,d I believe it is doable. I must admit that I just bring up this menu (by clicking on the X next to the info below the code): image https://user-images.githubusercontent.com/17723745/107182355-dd36a680-69dc-11eb-9348-24448ee28f5e.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pharo-project/pharo/issues/2794#issuecomment-774897090, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKL3V6OUBKH27CWKCWWUQ3S556CZANCNFSM4G5EL4MA.

[guillep]

Is there a way we can actionate this?

[macta]

yes - set the number to 4 - its not perfect, but will catch obvious abuse - below that threshold you have to use your own common sense.