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

[MSHARED-1224] Prefer JDK classes to Plexus utils #81

Merged
merged 10 commits into from
Apr 8, 2023
Merged

[MSHARED-1224] Prefer JDK classes to Plexus utils #81

merged 10 commits into from
Apr 8, 2023

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Mar 11, 2023

This completely removes Plexus utils from this package.

@elharo elharo requested a review from michael-o March 11, 2023 15:41
if ( !directory.isDirectory() )

List<Path> classFiles = Files.walk( directory.toPath() )
.filter( path -> path.toFile().getName().endsWith( ".class" ) )
Copy link
Member

Choose a reason for hiding this comment

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

Why not #getFileName()#toString()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Six of one, half dozen of another. I looked at that and saw that getFileName returned a Path instead of a String, which seemed wrong to me. But if you prefer I can change this.

Copy link
Member

Choose a reason for hiding this comment

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

Let's make consistently use NIO2.

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
Contributor Author

Choose a reason for hiding this comment

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

might have to revert that. It seems to break the integration tests on Windows in a way that indicates a real issue.

@elharo
Copy link
Contributor Author

elharo commented Mar 24, 2023

Trickier than I thought. The problem is that jars on Windows still use forward slash but directories use backslash.

@elharo elharo marked this pull request as ready for review March 24, 2023 15:14
Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

Please create issue for it.

@@ -99,7 +97,8 @@ private static void acceptJar( URL url, ClassFileVisitor visitor )
// ignore files like package-info.class and module-info.class
if ( name.endsWith( ".class" ) && name.indexOf( '-' ) == -1 )
{
visitClass( name, in, visitor );
// Even on Windows Jars use / as the separator character
Copy link
Member

Choose a reason for hiding this comment

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

The comment is redundant and wrong because this has nothing to do with Windows. REad the PKZIP implementation note: separators are always foward slashes regarding of the OS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the point. The previous version was using the system dependent line separator, which was why the test was failing on Windows but not Linux.

Copy link
Member

Choose a reason for hiding this comment

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

comment was changed

@slawekjaranowski slawekjaranowski changed the title Prefer JDK classes to Plexus utils [MSHARED-1224] Prefer JDK classes to Plexus utils Apr 7, 2023
@michael-o michael-o self-requested a review April 7, 2023 23:28
@elharo elharo merged commit 93f9e36 into master Apr 8, 2023
@elharo elharo deleted the plex branch April 8, 2023 10:41
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.

3 participants