Documentation adding and editing #5490
Conversation
|
@MartinWitt @SirYwell Hi has anyone looked at this PR yet? |
| /** | ||
| * This method retrieves a list of variables from a given parent CtElement instance. | ||
| * @param parent The parent element from which to retrieve variables. | ||
| * @return A list of variables found within the parent element. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * This method retrieves a list of variables from a given parent CtElement instance. | |
| * @param parent The parent element from which to retrieve variables. | |
| * @return A list of variables found within the parent element. | |
| */ |
We can skip documentation for private methods.
|
|
||
| /* | ||
| * This class scans for variables within a given element. | ||
| */ |
There was a problem hiding this comment.
| /* | |
| * This class scans for variables within a given element. | |
| */ |
We can skip for package-private class as well.
| /** | ||
| * Appends tabs or spaces to a StringBuilder based on the environment settings. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Appends tabs or spaces to a StringBuilder based on the environment settings. | |
| */ |
| /** | ||
| * Writes tabs if the condition is met. | ||
| * This method checks if tabs should be written, and if so, it writes them and resets the flag. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * Writes tabs if the condition is met. | |
| * This method checks if tabs should be written, and if so, it writes them and resets the flag. | |
| */ |
| /** | ||
| * @param c Character that needs to be checked for being space/line-break character | ||
| * @return true if character is space/line-break character | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * @param c Character that needs to be checked for being space/line-break character | |
| * @return true if character is space/line-break character | |
| */ |
|
|
||
| /** | ||
| * This method visits a list of statements and adds any variables it finds to the list. | ||
| * @param e The list of statements to visit. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * This method visits a list of statements and adds any variables it finds to the list. | |
| * @param e The list of statements to visit. | |
| */ |
I think we can remove all the docstring in the overridden methods in this class as they are not part of the public API. They are just public.
There was a problem hiding this comment.
Alright. Thanks for pointing out these corrections. I will rectify these and add new javadocs based your suggestions.
|
@monperrus you can merge it after approving the workflow. |
|
Hey, We had some internal discussion about this PR. Could you reduce this PR to at max 3 methods to make it easier to review? There are some Javadoc comments which seem wrong, and we can discuss them in depth easier if there are only a few changes. |
|
Hi @MartinWitt , I apologize if any of the comments seem wrong. I will work on reducing the number of methods covered by the javadocs down to 3, but I was curious if you want any specific methods or any specific class's methods to be a part of those 3. |
|
Hey @TheMarvelFan, Please have no hesitation to choose any method you like. Every added documentation is awesome. |
Methods whose documentation was retained: AstParentConsistencyChecker.java: scan() CacheBasedConflictFinder.java: hasNestedTypeConflict(), hasFieldConflict()
| @@ -13,6 +13,11 @@ | |||
| public class AstParentConsistencyChecker extends CtScanner { | |||
|
|
|||
| private CtElement parent; | |||
|
|
|||
| /** | |||
| * Scans a CtElement in the abstract syntax tree (AST) and enforces reference sharing across the AST. | |||
There was a problem hiding this comment.
I dont get where this method enforces references sharing.
There was a problem hiding this comment.
This method does not exactly enforce sharing references, but allows sharing reference across the Abstract Syntax Tree. I will correct this.
| @@ -28,7 +28,11 @@ public class CacheBasedConflictFinder { | |||
| typeRef = type.getReference(); | |||
| } | |||
|
|
|||
| /** returns true if the given name is a field name */ | |||
| /** | |||
| * Checks if a field with the given name conflicts with fields in the type's scope. | |||
There was a problem hiding this comment.
Explain what a conflict is.
There was a problem hiding this comment.
I will add the suggested changes.
|
Thank you for your contribution, but we do not believe that the documentation change in this PR is worth it at this stage. |
Referencing issue #1876
Added documentation for methods that I found undocumented, and also added param and return annotation for methods that didn't have them (referencing issue #3923), along with improving some present documentation that seemed unclear. This is a work in progress and I will keep adding commits to it, but I wanted someone to review whether this pull request so I can make sure whether this is the right way/standard to do it.
Thanks.