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.

Sign up for GitHub

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

Jump to bottom

[MSHARED-1090] Update module #11

Merged
merged 4 commits into from
Jun 28, 2022
Merged

[MSHARED-1090] Update module #11

merged 4 commits into from
Jun 28, 2022

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Jun 27, 2022 •
edited
Loading

Update module:

  • parent to POM 36
  • Maven level to 3.2.5
  • Java level to 8
  • drop m-shared-utils (deprecated method call replaced with what deprecation suggested)
  • drop legacy APIs like plexus logger, all the logger wrapper, settle on slf4j

https://issues.apache.org/jira/browse/MSHARED-1090

@cstamas
[MSHARED-1090] Update module
e2fb3dc 
Update module:
* parent to POM 36
* Maven level to 3.2.5
* Java level to 8
* drop m-shared-utils
* drop legacy APIs like plexus logger
@cstamas cstamas self-assigned this Jun 27, 2022
@cstamas cstamas mentioned this pull request Jun 27, 2022
Merged
@jorsol
Copy link

jorsol commented Jun 27, 2022

This change should probably bump the minor version.

@cstamas
Copy link
Member Author

cstamas commented Jun 27, 2022

Of course, but did not want to mix the two (there are a LOT of other changes already requiring it)

@cstamas
Copy link
Member Author

cstamas commented Jun 27, 2022

Hm, maybe we need NoopMessageSink to retain existing behaviour with defautl ctor... as now it will use sl4j logger and this is behaviour change from before when def ctor used.

@michael-o
Copy link
Member

Where is this module still used?

@cstamas
Copy link
Member Author

cstamas commented Jun 27, 2022

maven-jar-plugin for sure, unsure for others, but gh search reveals a LOT of uses across many (non our) plugins

@cstamas
Copy link
Member Author

cstamas commented Jun 27, 2022

https://github.com/search?l=Java&q=org.apache.maven.shared.model.fileset.util.FileSetManager&type=Code

@michael-o
Copy link
Member

Why do you I have the feeling that I have seen this with Plexus Utils too?!

@slachiewicz
Copy link
Member

Maybe we can just switch to pure slf4j?

@cstamas
Copy link
Member Author

cstamas commented Jun 28, 2022

Why do you I have the feeling that I have seen this with Plexus Utils too?!

This is "already seen" to me as well, no idea where but really looks familiar...

@slawekjaranowski
Copy link
Member

As @slachiewicz said - why not use slf4j?

There is changed from plexus Logger to slf4j Logger so change will be incompatible.

michael-o
michael-o requested changes Jun 28, 2022
View reviewed 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.

Please, can we throw out this ugly wrapper around wrappe around SLF4J even if we need to bump the major version?!

pom.xml Outdated Show resolved Hide resolved
@cstamas
Copy link
Member Author

cstamas commented Jun 28, 2022

Tidied up completely

@cstamas cstamas requested a review from michael-o June 28, 2022 20:19
michael-o
michael-o requested changes Jun 28, 2022
View reviewed changes
src/main/java/org/apache/maven/shared/model/fileset/util/FileSetManager.java
* @param log The mojo log instance
*/
public FileSetManager( Log log )
public FileSetManager( Logger logger, boolean verbose )
Copy link
Member

Choose a reason for hiding this comment

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

I think this ctor should be deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Disagree, this ctor controls what logger this class will use and verbosity (not level)

src/main/java/org/apache/maven/shared/model/fileset/util/FileSetManager.java
*/
public FileSetManager( Logger log )
public FileSetManager( Logger logger )
Copy link
Member

Choose a reason for hiding this comment

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

I think this ctor should be deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Disagree, this ctor controls what logger this class will use.

src/main/java/org/apache/maven/shared/model/fileset/util/FileSetManager.java
@@ -285,18 +241,18 @@ public void delete( FileSet fileSet, boolean throwsError )
{
if ( fileSet.isFollowSymlinks() || !isSymlink( file ) )
{
if ( verbose && messages != null )
if ( verbose )
Copy link
Member

Choose a reason for hiding this comment

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

This completely circumvents log levels

src/main/java/org/apache/maven/shared/model/fileset/util/FileSetManager.java
}

removeDir( file, fileSet.isFollowSymlinks(), throwsError, warnMessages );
}
else
{ // delete a symlink to a directory without follow
if ( verbose && messages != null )
if ( verbose )
Copy link
Member

Choose a reason for hiding this comment

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

ditto

src/main/java/org/apache/maven/shared/model/fileset/util/FileSetManager.java
@@ -316,9 +272,9 @@ public void delete( FileSet fileSet, boolean throwsError )
}
else
{
if ( verbose && messages != null )
if ( verbose )
Copy link
Member

Choose a reason for hiding this comment

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

ditto

src/main/java/org/apache/maven/shared/model/fileset/util/FileSetManager.java
@@ -380,9 +336,9 @@ private Set<String> findDeletablePaths( FileSet fileSet )

private Set<String> findDeletableDirectories( FileSet fileSet )
{
if ( verbose && messages != null )
if ( verbose )
Copy link
Member

Choose a reason for hiding this comment

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

ditto

src/main/java/org/apache/maven/shared/model/fileset/util/FileSetManager.java
@@ -398,31 +354,31 @@ private Set<String> findDeletableDirectories( FileSet fileSet )

if ( !fileSet.isFollowSymlinks() )
{
if ( verbose && messages != null )
if ( verbose )
Copy link
Member

Choose a reason for hiding this comment

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

ditto

src/main/java/org/apache/maven/shared/model/fileset/util/FileSetManager.java
@@ -437,9 +393,9 @@ private Set<String> findDeletableDirectories( FileSet fileSet )

private Set<String> findDeletableFiles( FileSet fileSet, Set<String> deletableDirectories )
{
if ( verbose && messages != null )
if ( verbose )
Copy link
Member

Choose a reason for hiding this comment

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

ditto

src/main/java/org/apache/maven/shared/model/fileset/util/FileSetManager.java
@@ -455,31 +411,31 @@ private Set<String> findDeletableFiles( FileSet fileSet, Set<String> deletableDi

if ( !fileSet.isFollowSymlinks() )
{
if ( verbose && messages != null )
if ( verbose )
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@cstamas
Copy link
Member Author

cstamas commented Jun 28, 2022

re verbose: disagree. This is "verbose", will tell more, that's all. Log level is INFO

@cstamas
Copy link
Member Author

cstamas commented Jun 28, 2022

verbose is orthogonal to log level. That's all IMHO

@cstamas
Copy link
Member Author

cstamas commented Jun 28, 2022

General: this is a utility class, is NOT a component.

@cstamas cstamas requested a review from michael-o June 28, 2022 20:46
@cstamas cstamas merged commit c7153b1 into master Jun 28, 2022
@cstamas cstamas deleted the update-module branch June 28, 2022 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-o michael-o michael-o approved these changes

@hboutemy hboutemy Awaiting requested review from hboutemy

@elharo elharo Awaiting requested review from elharo

@slachiewicz slachiewicz Awaiting requested review from slachiewicz

Assignees

@cstamas cstamas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cstamas @jorsol @michael-o @slachiewicz @slawekjaranowski