Skip to content

Conversation

@slawekjaranowski
Copy link
Member

No description provided.

@slawekjaranowski slawekjaranowski added enhancement Improving an existing feature. dependencies Pull requests that update a dependency file labels Sep 28, 2024
@michael-o
Copy link
Member

Will take a look...

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

There are several problems in src/main/java/org/codehaus/mojo/taglist/TagListReport.java:

src/main/java/org/codehaus/mojo/taglist/ReportGenerator.java:

  • It should be called ...Renderer and inherit from AbstractMavenReportRenderer
  • Maybe move when the move has been done

@slawekjaranowski
Copy link
Member Author

@michael-o - next part of changes ...

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Will import the code into my IDE and do another review.

* The Locale of the files to analyze.
*/
private final Locale locale;
private final Locale sourceFileLocale;
Copy link
Member

Choose a reason for hiding this comment

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

sourceFilesLocale, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a sourceFileLocale parameter of Mojo report ... so let to be consistent

Copy link
Member

Choose a reason for hiding this comment

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

Yes, both need to be consistent with a TODO.

@michael-o
Copy link
Member

Is this on purpose:

OutputStreamWriter output = new OutputStreamWriter(fos, getInputEncoding())) {

If yes, it should have a comment...

@michael-o
Copy link
Member

Regarding:

private ResourceBundle getBundle(Locale locale) {
return ResourceBundle.getBundle(
"taglist-report", locale, this.getClass().getClassLoader());
}

Though it is not wrong, the canonical way is to use the I18n class via DI. See PMDReport.java

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Last round of comments

@michael-o
Copy link
Member

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

This good enough, let's go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file enhancement Improving an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants