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

String concatenation critique is too strict #2794

Open
macta opened this Issue Mar 11, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@macta
Copy link
Contributor

macta commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Contributor Author

macta commented Mar 11, 2019

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

This comment has been minimized.

Copy link
Member

astares commented Mar 12, 2019

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

This comment has been minimized.

Copy link
Contributor

bencoman commented Mar 12, 2019

@macta

This comment has been minimized.

Copy link
Contributor Author

macta commented Mar 12, 2019

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.