Improve Javadoc. #3841
Improve Javadoc. #3841
Conversation
|
Initial/Pre-Review Thoughts I didn't actually read through the javadoc changes, so if there's questions during the main review feel free to ask. Questions I have:
Potential risks:
What could make the full review difficult:
|
I did. Just improvement and add new one. |
…ernalg/liquibase into arturobernalg-afeature/improve-javadoc
MalloD12
left a comment
There was a problem hiding this comment.
Approved.
Review and testing results:
This looks ok to me. I have only made a very few minor tweaks to some javadocs. Thanks @arturobernalg.
Things to be aware of:
- None
Things to worry about:
- None
…ernalg/liquibase into arturobernalg-afeature/improve-javadoc
| * | ||
| * @param contexts The contexts to filter for. | ||
| * @param labelExpression The labels to filter for. | ||
| * @param collectAllReasons A flag to control whether all skip reasons are accumulated. |
There was a problem hiding this comment.
I do not see the collectAllReasons parameter in the method definition. Additionally, the @return list is not complete (missing ShouldRunChangeSetFilter, ContextChangeSetFilter, LabelChangeSetFilter, DbmsChangeSetFilter and IgnoreChangesetFilter).
The "vbnet copy code" words seem random??? Perhaps a mistake?
CC @MalloD12 , @arturobernalg
There was a problem hiding this comment.
Hey Erz, I'm going to update this as I saw it collectAllReasons wasn't there previously:
/**
*
* Return a ChangeLogIterator constructed with standard filters
*
* @param contexts Contexts to filter for
* @param labelExpression Labels to filter for
* @param changeLog The changelog to process
*
* @return ChangeLogIterator
* @throws DatabaseException
*
*/
protected ChangeLogIterator getStandardChangelogIterator(Contexts contexts, LabelExpression labelExpression,
DatabaseChangeLog changeLog) throws DatabaseException {
return new ChangeLogIterator(changeLog,
new ShouldRunChangeSetFilter(database),
new ContextChangeSetFilter(contexts),
new LabelChangeSetFilter(labelExpression),
new DbmsChangeSetFilter(database),
new IgnoreChangeSetFilter());
}
When talk about the missing filters in the @return do you mean we should be adding them in the javadoc? It specifies a ChangelogIterator instance is returned and that's what the method is returning.
liquibase-standard/src/main/java/liquibase/hub/core/StandardHubService.java
Outdated
Show resolved
Hide resolved
| * @throws DatabaseException | ||
| * Executes the prepared statement created by the given factory. | ||
| * | ||
| * @param factory a factory for creating a <code>PreparedStatement</code> object. |
There was a problem hiding this comment.
Should <code>PreparedStatement</code> be `{@code PreparedStatement}'?
There was a problem hiding this comment.
If I'm not wrong they both have the same purpose, even {@code} (coming from Java5) is older than which I think will format better indentations and line breaks.
liquibase-standard/src/main/java/liquibase/util/StringUtil.java
Outdated
Show resolved
Hide resolved
XDelphiGrl
left a comment
There was a problem hiding this comment.
I've left a few comments for (potential) requested changes.
I also validated that we're not making any changes to serialization in JavaDoc annotations to prevent regressions like https://github.com/liquibase/liquibase/pull/4161/files.
…ure/improve-javadoc # Conflicts: # liquibase-standard/src/main/java/liquibase/hub/core/StandardHubService.java
|
@XDelphiGrl I have made a few updates based on your comments. Can you please re-review them? And please let me know if you think there are more requested changes. Thanks, |
|
Thanks for the code changes, @MalloD12. I'm going to wait for the functional tests to run (since there is a lot of change due to the Hub refactoring work). When those pass, I'll do the final review/approval. |
|
For whatever reason, the test harness is lingering in the "pending" state. I've kicked off a manual build here: https://github.com/liquibase/liquibase-test-harness/actions/runs/4996652294. PASSED I excluded the HSQL and Firebird databases as they are failing due to Titan errors. |
XDelphiGrl
left a comment
There was a problem hiding this comment.
This is a JavaDoc cleanup PR that improves @param and @return documentation for many methods.
- No serializable annotations are modified in this PR (which is good!)
- Functional tests and test harness executions are passing.
- No additional testing required.
APPROVED
Impact
Description