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

Issue #5832: Add javadoc and xdoc Example for MemberName #6697

Merged
merged 1 commit into from
May 10, 2019

Conversation

tsunghanjacktsai
Copy link
Contributor

@tsunghanjacktsai tsunghanjacktsai force-pushed the member_name branch 4 times, most recently from 93c28be to 96084cb Compare April 27, 2019 14:12
@tsunghanjacktsai
Copy link
Contributor Author

@romani @rnveach Please have a look. I wanna know why Travis-ci is failed. I don't know what the meaning is inside the log.

@rnveach
Copy link
Member

rnveach commented Apr 27, 2019

Travis is having some issue right now with openjdk 9+, so you can ignore those errors.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

item to improve:

@@ -56,16 +56,74 @@
* <pre>
* &lt;module name="MemberName"/&gt;
* </pre>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please put example of applyToXxxxxx to configs and examples.
for example applyToPublic and applyToProtected to first example and applyToPackage and applyToPrivate to second example.

users love examples so mush that they skip reading of documentation at all. I think you noticed the same with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made some little changes in second example.

However, I think in the first example, users could see how the default config works. In the third example, I put the applyToPublic & applyToPrivate which could combine with the applyToPackage & applyToProtected in the second example.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -56,16 +56,74 @@
* <pre>
* &lt;module name="MemberName"/&gt;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace quotes with &quot;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if no escaping done ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, so do I need to change this one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if no escaping done ?

I do not know. But it will be better if we use one style per file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal preference is to avoid escaping if possible, as it is made code less readable.

@rnveach , what is your position on this ?
It is better to make decision on this for future updates.

In this PR, let's escape, to be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

@rnveach rnveach Apr 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is your position on this ?

I have no real preference.
@pbludov has been escaping these in all his PRs and I mentioned in one if he wants this done everywhere, he should make it part of the test otherwise new committers won't do it. See #6662 (comment) .

What happens if no escaping done ?

Nothing as long as we escape the <s are escaped. These were real " for the longest time. You only really need to escape if you are using it inside another quote like an attribute or text area or such.
Edit: I was thinking this was the xdoc. I don't see how this can affect the javadoc at all, again as long as the <s are escaped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbludov,

I do not know.

Does it make sense to do massive search-replace to un escape double braces ?

@tsunghanjacktsai tsunghanjacktsai force-pushed the member_name branch 2 times, most recently from 7222a80 to 8201ee0 Compare April 28, 2019 23:07
@tsunghanjacktsai
Copy link
Contributor Author

@romani So is there any other problem in this PR?

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbludov , please finalize

private int NUM4; // violation, name 'NUM4'
// must match pattern '^[a-z][a-zA-Z0-9]*$'
}
</pre>
<p>
An example of how to configure the check for names that begin with
<code>&quot;m&quot;</code>, followed by an upper case letter, and then letters and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description was not updated when applyTo were added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* // must match pattern '^[a-z][a-zA-Z0-9]*$'
* private int NUM4; // violation, name 'NUM4'
* // must match pattern '^[a-z][a-zA-Z0-9]*$'
* }
* </pre>
* <p>
* An example of how to configure the check for names that begin with
* {@code "m"}, followed by an upper case letter, and then letters and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

@pbludov pbludov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK to merge when CI passes.

@romani romani merged commit 1b6846d into checkstyle:master May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants