1. Fixed a bug when adding extra spaces after the parameter name. 2. Fixed a bug when using comment-like text in string literals. #1301

PeJetuz · 2024-07-04T18:03:54Z

@pnatashap @yegor256
Please review, this should resolve the issue 1079.

@param

The @param regex allows varying numbers of spaces before and after the @param tag, while the @SInCE tag only allows one space before and one after it.

The error no longer appears for string literals similar to comments.

PeJetuz · 2024-08-15T07:22:52Z

@yegor256 @pnatashap ping

yegor256 · 2024-08-15T14:20:25Z

@PeJetuz can you please merge master into your branch, since there are merge conflicts now?

PeJetuz · 2024-08-27T13:45:26Z

@yegor256 done.

PeJetuz · 2024-09-16T14:57:52Z

@yegor256 Hi!, the branch has been updated.

yegor256 · 2024-09-18T05:26:02Z

@volodya-lombrozo can you please help and review this pull request? It's pretty large and I'm afraid of merging it without a proper review.

volodya-lombrozo

@PeJetuz I have left just a few small suggestions. Also, it would be nice to have a short description of the changes made in this PR description; otherwise, it's hard to trace the original problem you're trying to pinpoint. Anyway, thanks so much for your contribution!

volodya-lombrozo · 2024-09-19T07:48:28Z

qulice-checkstyle/src/main/java/com/qulice/checkstyle/RequiredJavaDocTag.java

+     * @param patt Pattern for checking the contents of a tag in a string.
+     * @param rep Reference to a method for writing a message to the log.
+     */
+    RequiredJavaDocTag(final String name, final Pattern patt,


@PeJetuz NIT: What do you think about placing all the parameters either on one line, or each parameter in a separate line for the sake of consistency?

RequiredJavaDocTag(final String name, final Pattern patt, final Reporter rep) {

or

RequiredJavaDocTag( final String name, final Pattern patt, final Reporter rep ) {

The same is relevant for all the rest methods in this class.
I understand, that the format you used comes from the old classes from this repository, so it's not really important and you can leave it as is.

@volodya-lombrozo Thanks! Done.

volodya-lombrozo · 2024-09-19T08:02:24Z

qulice-checkstyle/src/test/java/com/qulice/checkstyle/RequiredJavaDocTagTest.java

+
+    @Test
+    void success() {
+        final String[] lines = {" *", " * @since 0.3", " *"};


@PeJetuz NIT: Many redundant variables are used in this test. Fore example, you can remove lines variables entirely.

There is the explanation why it's bad: https://www.yegor256.com/2015/09/01/redundant-variables-are-evil.html

Also, there is a study that emphasises significant negative impact of a hight number of identifiers in the code.

@volodya-lombrozo Thanks! Done.

volodya-lombrozo · 2024-09-19T08:32:03Z

qulice-checkstyle/src/test/resources/com/qulice/checkstyle/ValidSingleLineCommentCheck.java

+
+    /**
+     * Valid multi line literal.
+     * @checkstyle ArrayTrailingCommaCheck (6 lines)


@PeJetuz Why do you need to add ArrayTrailingCommaCheck here?

@volodya-lombrozo Thanks! I added a description in the comments to the methods. Is this ok?

@PeJetuz Maybe we can just remove this ArrayTrailingCommaCheck and add one more comma to the last element?

public static final String[] MULTILINE_LITERAL = { " /**", " * @since 0.3.4.4.", " **/", " /**", " * @since 0.3.4.4.", " **/", };

In this case you don't need to suppress this violation and to add explanation to it.

volodya-lombrozo · 2024-09-19T08:48:13Z

...e-checkstyle/src/test/resources/com/qulice/checkstyle/ChecksTest/CommentCheck/violations.txt

@@ -0,0 +1,5 @@
+6:This kind of comment is not allowed.
+9:First sentence in a comment should start with a capital letter.


@PeJetuz Could you help me here, please? I don't clearly understand why we have exactly this line numbers 6,9,9,15,19.

Line 6 - /* C-style comment */ - OK
Line 9 - /** first sentence in a comment should start with a capital letter */ - OK
Line 9 - Why did you duplicate it?
Line 15 - The end of the incorrect java doc, but not the line itself (why not the 14 line?)
Line 19 - Why not the 18?

@volodya-lombrozo Thanks! Duplication occurs because we have this check triggered twice, once in the class com.qulice.checkstyle.MultiLineCommentCheck and the second time in the class com.qulice.checkstyle.SingleLineCommentCheck.
Unfortunately, I don't know where it would be better to place the description of this case.

volodya-lombrozo · 2024-09-19T09:38:07Z

qulice-checkstyle/src/main/java/com/qulice/checkstyle/MultiLineCommentCheck.java

+        }
+    }
+
+    public void setFormat(final String fmt) {


@PeJetuz I understand why you need to add setters, but in general it is a bad practice.
I suggest to add at least some javadocs that would explain why we need them and why we can't remove them from the code base. It will preserve them from the future removal. The same is relevant for SingleLineCommentCheck

@volodya-lombrozo Thanks! I added a description in the comments to the methods.

volodya-lombrozo · 2024-09-19T09:44:10Z

qulice-checkstyle/src/main/java/com/qulice/checkstyle/MultiLineCommentCheck.java

+    @Override
+    public void visitToken(final DetailAST ast) {
+        if (ast.getType() == TokenTypes.BLOCK_COMMENT_BEGIN) {
+            this.line = new StringBuilder(ast.getText());


@PeJetuz Why do you need to initialise this.line field exactly here? Maybe we can initialise it directly in the constructor or in the field declaration?

private final StringBuilder line = new StringBuilder();

By doing this you will be able just to append text to it. First of all, this should simplify the code, and what is the most important, you will make this.line field immutable. How Immutability Helps.

@volodya-lombrozo Thanks! I described why we reinitialize the variable in the comment when declaring it. Unfortunately, I didn't find a place where it is done correctly. Is this ok?

volodya-lombrozo · 2024-09-19T09:48:14Z

qulice-checkstyle/src/test/java/com/qulice/checkstyle/RequiredJavaDocTagTest.java

+    }
+
+    @Test
+    void successWithSpaces() {


@PeJetuz NIT: I would highly recommend to use @ParameterizedTest from JUnit 5. This would allow to remove similar tests and consider more test cases. More about this type of testing is here.

@volodya-lombrozo Thanks! Done.

PeJetuz · 2024-09-20T08:59:14Z

@volodya-lombrozo Thank you! I'll read the articles and fix.

PeJetuz · 2024-10-01T13:29:58Z

@PeJetuz I have left just a few small suggestions. Also, it would be nice to have a short description of the changes made in this PR description; otherwise, it's hard to trace the original problem you're trying to pinpoint. Anyway, thanks so much for your contribution!

@volodya-lombrozo Thank you very much! I changed the commit message to describe the issues fixed. Is this okay?

volodya-lombrozo

@PeJetuz Thank you for the clarification. It's almost finished. I've just left several small comments.

volodya-lombrozo · 2024-10-03T09:45:41Z

qulice-checkstyle/src/test/resources/com/qulice/checkstyle/ValidSingleLineCommentCheck.java

+
+    /**
+     * Valid multi line literal.
+     * @checkstyle ArrayTrailingCommaCheck (6 lines)


@PeJetuz Maybe we can just remove this ArrayTrailingCommaCheck and add one more comma to the last element?

public static final String[] MULTILINE_LITERAL = { " /**", " * @since 0.3.4.4.", " **/", " /**", " * @since 0.3.4.4.", " **/", };

In this case you don't need to suppress this violation and to add explanation to it.

volodya-lombrozo · 2024-10-03T10:11:51Z

qulice-checkstyle/src/main/resources/com/qulice/checkstyle/checks.xml

+    <module name="com.qulice.checkstyle.MultiLineCommentCheck">
+      <property name="format" value=" */\*\* +[^A-Z\{ ][\W\s\S]*"/>
+      <property name="message" value="First sentence in a comment should start with a capital letter."/>
+      <property name="id" value="FirstLetterInCommentShouldBeCapital3"/>


@PeJetuz I have some doubts about id properties here. Can we just omit them? Or at least to give them more meaningful names. FirstLetterInCommentShouldBeCapital2 -> FirstLetterInMultiLineCommenWithAsterisk. Or something like this. FirstLetterInCommentShouldBeCapital2 and FirstLetterInCommentShouldBeCapital3 look strange to me.

@volodya-lombrozo Thanks for your help with ArrayTrailingCommaCheck!
I renamed id and changed the message for this check. Thanks for pointing this out, I think the message should be more friendly now.

volodya-lombrozo · 2024-10-03T10:20:49Z

qulice-checkstyle/src/main/java/com/qulice/checkstyle/MultiLineCommentCheck.java

+     * during the class under test and the field is reinitialized with a new object.
+     */
+    @SuppressWarnings("PMD.AvoidStringBufferField")
+    private StringBuilder line;


@PeJetuz It seems, that the line is not an appropriate name of variable here. Since it's a "MultilineComment", I would give it lines or text name. What do you think?

@volodya-lombrozo Absolutely agree. Fixed.

volodya-lombrozo · 2024-10-03T10:37:11Z

qulice-checkstyle/src/main/resources/com/qulice/checkstyle/checks.xml

+    </module>
+    <module name="com.qulice.checkstyle.MultiLineCommentCheck">
+      <property name="format" value=" */\*\*\W+\* +[^A-Z\{ ][\W\s\S]*"/>
+      <property name="message" value="First sentence in a comment should start with a capital letter."/>


@PeJetuz I would highly recommend you to change this error message:

First sentence in a comment should start with a capital letter.

->

First sentence in a multiline comment should start with a capital letter.

It will greatly help to distinct these two cases. The same you can do in tests.

@volodya-lombrozo Thanks! I've changed the messages so they should be more friendly now.

@param

* @param line Line position. * @param column Column position. 2. Fixed a bug when using comment-like text in string literals: final String comment = "/* some text */"; Fixes for issue yegor256#1079

volodya-lombrozo

@PeJetuz Looks good to me. Thank you for the contribution!
@yegor256 Could you merge these changes, please?

yegor256 · 2024-10-17T05:41:13Z

@rultor merge

rultor · 2024-10-17T05:43:09Z

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here.

rultor · 2024-10-17T05:55:10Z

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 11min).

Vladimir.Shapkin added 2 commits July 4, 2024 17:42

Fixed different behavior between @param and @SInCE

8c8068c

The @param regex allows varying numbers of spaces before and after the @param tag, while the @SInCE tag only allows one space before and one after it.

Fixed issue 1079

345e149

The error no longer appears for string literals similar to comments.

volodya-lombrozo suggested changes Sep 19, 2024

View reviewed changes

PeJetuz force-pushed the fix_space_in_tags branch 3 times, most recently from 7259ac0 to 4844907 Compare October 1, 2024 12:58

PeJetuz changed the title ~~Fix issue 1079~~ 1. Fixed a bug when adding extra spaces after the parameter name 2. Fixed a bug when using comment-like text in string literals. Oct 1, 2024

PeJetuz changed the title ~~1. Fixed a bug when adding extra spaces after the parameter name 2. Fixed a bug when using comment-like text in string literals.~~ 1. Fixed a bug when adding extra spaces after the parameter name. 2. Fixed a bug when using comment-like text in string literals. Oct 1, 2024

volodya-lombrozo suggested changes Oct 3, 2024

View reviewed changes

1. Fixed a bug when adding extra spaces after the parameter name

dfc2c62

* @param line Line position. * @param column Column position. 2. Fixed a bug when using comment-like text in string literals: final String comment = "/* some text */"; Fixes for issue yegor256#1079

PeJetuz force-pushed the fix_space_in_tags branch from 4844907 to dfc2c62 Compare October 16, 2024 15:07

volodya-lombrozo approved these changes Oct 17, 2024

View reviewed changes

rultor merged commit e8f1762 into yegor256:master Oct 17, 2024
8 checks passed

		@@ -0,0 +1,5 @@
		6:This kind of comment is not allowed.
		9:First sentence in a comment should start with a capital letter.

1. Fixed a bug when adding extra spaces after the parameter name. 2. Fixed a bug when using comment-like text in string literals. #1301

1. Fixed a bug when adding extra spaces after the parameter name. 2. Fixed a bug when using comment-like text in string literals. #1301

Conversation

PeJetuz commented Jul 4, 2024

PeJetuz commented Aug 15, 2024

yegor256 commented Aug 15, 2024

PeJetuz commented Aug 27, 2024

PeJetuz commented Sep 16, 2024

yegor256 commented Sep 18, 2024

volodya-lombrozo 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

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

PeJetuz Oct 1, 2024 • 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

PeJetuz Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeJetuz commented Sep 20, 2024 • edited Loading

PeJetuz commented Oct 1, 2024

volodya-lombrozo 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

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

volodya-lombrozo left a comment

Choose a reason for hiding this comment

yegor256 commented Oct 17, 2024

rultor commented Oct 17, 2024

rultor commented Oct 17, 2024

PeJetuz Oct 1, 2024 •

edited

Loading

PeJetuz Oct 1, 2024 •

edited

Loading

PeJetuz commented Sep 20, 2024 •

edited

Loading