fix: fix qodana warning with proper call to Array.toString() #4536

MartinWitt · 2022-01-18T20:55:28Z

Change Log

The following bad smells are refactored:

ArraysToString

array.toString() is not the best way to print an array. Use Arrays.toString(array) instead.

https://rules.sonarsource.com/java/RSPEC-2116

The following has changed in the code:

ArraysToString

Replaced str.toString() with Arrays.toString(str).

The following has changed in the code: Replaced str.toString() with Arrays.toString(str).

algomaster99 · 2022-01-18T21:01:20Z

@MartinWitt where can I see the list of all Qodana warnings? We have a tool that can automatically repair such types of warnings - Sorald. You can see the list of rules it handles as of now here.

EDIT: I guess you have already contributed to it ASSERT-KTH/sorald#368 :)

MartinWitt · 2022-01-18T21:04:08Z

Do you want a list of all we have or all qodana has?
For all Qodana(IntelliJ) supports, see. https://www.jetbrains.com/help/idea/list-of-java-inspections.html

algomaster99 · 2022-01-18T21:07:28Z

@MartinWitt Both would be good. I will see if we can fix any violations using Sorald.

MartinWitt · 2022-01-18T21:11:49Z

For all we have, see here https://github.com/INRIA/spoon/pull/4481/files (we disabled this view in a recent PR) or run sudo docker run --rm -it -p 8080:8080 -v $PWD/:/data/project/ jetbrains/qodana-jvm-community:2021.3 --show-report
For all rules, see here https://www.jetbrains.com/help/idea/list-of-java-inspections.html
Maybe you can open the project in IntelliJ and run qodana/the inspections there.

algomaster99

Delete the new line and then we can merge.

algomaster99 · 2022-01-19T10:07:32Z

src/main/java/spoon/support/sniper/internal/ElementSourceFragment.java

@@ -7,6 +7,7 @@
 */
 package spoon.support.sniper.internal;

+


You accidentally added this new line.

Yes, I still have problems of added newlines while sniper printing. I removed it.

monperrus · 2022-01-19T12:54:31Z

src/main/java/spoon/support/sniper/internal/ElementSourceFragment.java

@@ -720,7 +721,7 @@ private void onCharSequence(CharType type, StringBuilder buff, Consumer<SourceFr
 			}
 			// nothing has matched, best effort token
 			if (longestMatcher == null) {
-				consumer.accept(new TokenSourceFragment(str.toString(), TokenType.CODE_SNIPPET));
+				consumer.accept(new TokenSourceFragment(Arrays.toString(str), TokenType.CODE_SNIPPET));


very confusing to have an array named str, charArray is better?

Yes, I renamed it

monperrus · 2022-01-19T13:32:44Z

src/main/java/spoon/support/sniper/internal/ElementSourceFragment.java

 					longestMatcher = strMatcher.getLonger(longestMatcher);
 				}
 			}
 			// nothing has matched, best effort token
 			if (longestMatcher == null) {
-				consumer.accept(new TokenSourceFragment(str.toString(), TokenType.CODE_SNIPPET));
+				consumer.accept(new TokenSourceFragment(Arrays.toString(charArray), TokenType.CODE_SNIPPET));


this looks like this best-effort token never worked. I guess we don't have any single test here :)?

Looking at the coverage, there should exist a testcase for this line https://coveralls.io/builds/45751341/source?filename=src%2Fmain%2Fjava%2Fspoon%2Fsupport%2Fsniper%2Finternal%2FElementSourceFragment.java#L724. But I have not found a way in the UI to find it.

Looking at the coverage, there should exist a testcase for this line coveralls.io/builds/45751341/source?filename=src%2Fmain%2Fjava%2Fspoon%2Fsupport%2Fsniper%2Finternal%2FElementSourceFragment.java#L724. But I have not found a way in the UI to find it.

argh, this is #4541

anyway, I'd be in favor in writing an assertion on the content of this token. Otherwise, we fix broken code by something which may be buggy as well

would you be able to add such an assertion?

After some debugging, I found the testcase

spoon/src/test/java/spoon/test/prettyprinter/TestSniperPrinter.java

Line 187 in f98e33b

public void testPrintLocalVariableDeclaration2() {

using this line with a string 0,

I believe the current input token is simply wrong.

Agree, that's why we need a new test or a new assertion to assert the actual correct content. Would you write this?

I can only trigger this line with joint declarations but currently the sniper is not able to work with them, see #3386. It looks like either this token was never relevant or is needed in a future version and we can't great asserts for the value currently.

If we are unable to specify this line, then the best option is to simply remove it. Could you give it a try?

It looks like the token is for some strange reasons important for the sniper printing and removing it adds unwanted whitespaces. So we more or less have to keep it and fix this after fixing the linked problem.

ack, then let's revert this last commit and proceed with the change, it's not harmful. we replace code we don't understand by code we still don't understand but less confusing.

monperrus · 2022-02-05T08:03:27Z

Thanks @MartinWitt

MartinWitt added 2 commits January 18, 2022 21:52

refactor(ElementSourceFragment):

2bc723f

The following has changed in the code: Replaced str.toString() with Arrays.toString(str).

reduce diff

bf31f49

algomaster99 approved these changes Jan 19, 2022

View reviewed changes

monperrus reviewed Jan 19, 2022

View reviewed changes

change naming of variable

aa4f970

MartinWitt force-pushed the refactor/qodana/ElementSourceFragment branch from 214c44c to aa4f970 Compare January 19, 2022 13:17

monperrus reviewed Jan 19, 2022

View reviewed changes

MartinWitt added 3 commits February 3, 2022 10:51

Update ElementSourceFragment.java

ff7d17e

Update ElementSourceFragment.java

27eebdb

fix build

caa9074

monperrus changed the title ~~refactor: fix qodana warning array.toString()~~ fix: fix qodana warning with proper call to Array.toString() Feb 5, 2022

monperrus merged commit f71e9e6 into INRIA:master Feb 5, 2022

monperrus mentioned this pull request Mar 14, 2022

WIP release: next release #4636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix qodana warning with proper call to Array.toString() #4536

fix: fix qodana warning with proper call to Array.toString() #4536

MartinWitt commented Jan 18, 2022

algomaster99 commented Jan 18, 2022 •

edited

Loading

MartinWitt commented Jan 18, 2022 •

edited

Loading

algomaster99 commented Jan 18, 2022

MartinWitt commented Jan 18, 2022

algomaster99 left a comment

algomaster99 Jan 19, 2022

MartinWitt Jan 19, 2022

monperrus Jan 19, 2022

MartinWitt Jan 19, 2022 •

edited

Loading

monperrus Jan 19, 2022

MartinWitt Jan 19, 2022

monperrus Jan 20, 2022

monperrus Jan 20, 2022

MartinWitt Jan 21, 2022

monperrus Jan 26, 2022

MartinWitt Feb 2, 2022

monperrus Feb 3, 2022

MartinWitt Feb 3, 2022

monperrus Feb 4, 2022

monperrus commented Feb 5, 2022

fix: fix qodana warning with proper call to Array.toString() #4536

fix: fix qodana warning with proper call to Array.toString() #4536

Conversation

MartinWitt commented Jan 18, 2022

Change Log

ArraysToString

The following has changed in the code:

ArraysToString

algomaster99 commented Jan 18, 2022 • edited Loading

MartinWitt commented Jan 18, 2022 • edited Loading

algomaster99 commented Jan 18, 2022

MartinWitt commented Jan 18, 2022

algomaster99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinWitt Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monperrus commented Feb 5, 2022

algomaster99 commented Jan 18, 2022 •

edited

Loading

MartinWitt commented Jan 18, 2022 •

edited

Loading

MartinWitt Jan 19, 2022 •

edited

Loading