Skip to content

Issue 124 add java nio path support#126

Merged
lukasj merged 7 commits into
jakartaee:masterfrom
rmcdouga:issue_124_add_java_nio_path_support
Apr 30, 2023
Merged

Issue 124 add java nio path support#126
lukasj merged 7 commits into
jakartaee:masterfrom
rmcdouga:issue_124_add_java_nio_path_support

Conversation

@rmcdouga

@rmcdouga rmcdouga commented Apr 2, 2023

Copy link
Copy Markdown
Contributor

PR supporting Issue #124

rmcdouga added 2 commits April 1, 2023 10:14
Changed the internal implementation to use Path instead of File.
@rmcdouga

rmcdouga commented Apr 2, 2023

Copy link
Copy Markdown
Contributor Author

I see the builds are failing due to Javadoc. I will correct this. Also, it seems like the eca issue is related to the multiple email addresses I have on my GitHub account. I have accepted the ECA but the automated test seems to be confused. I will see what can be done.

@lukasj lukasj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in general, looks good, yet some fine-tunning is needed:

  • avoid using tabs and stick with current formatting
  • reformatting comments as part of the change does not make reviewer's life easier
  • copyright year should be updated
  • change should be noted in the spec document, at least in the changelog

should marking File variants as deprecated be considered or do you propose to just keep them as they are for now?

Comment thread .gitignore Outdated
Comment thread api/src/main/java/jakarta/activation/MimetypesFileTypeMap.java Outdated
Comment thread api/src/main/java/jakarta/activation/FileDataSource.java Outdated
Comment thread api/src/main/java/jakarta/activation/MimetypesFileTypeMap.java Outdated
- Also updated to latest maven javadoc plugin.
@rmcdouga

rmcdouga commented Apr 3, 2023

Copy link
Copy Markdown
Contributor Author

avoid using tabs and stick with current formatting

Should I go back and change it back to what it was. Sorry, the formatting style seemed very old-school to me, so I thought I was making things better but I understand the value of consistency. I am happy to change it back.

reformatting comments as part of the change does not make reviewer's life easier

Understood.

copyright year should be updated

Will do.

change should be noted in the spec document, at least in the changelog

Will do.

should marking File variants as deprecated be considered or do you propose to just keep them as they are for now?

This is a tough call. Personally, I would deprecate them however the JDK maintainers have seen fit not to deprecate File even though it has been superseded. I decided to follow the JDK lead and since it is not deprecated in the JDK, to leave it undeprecated in this API. As a compromise, I put a note in the Javadoc indicating that the Path version should be preferred.

@lukasj

lukasj commented Apr 4, 2023

Copy link
Copy Markdown
Contributor

avoid using tabs and stick with current formatting

Should I go back and change it back to what it was. Sorry, the formatting style seemed very old-school to me, so I thought I was making things better but I understand the value of consistency. I am happy to change it back.

yes, please - avoid tabs for new stuff. The codebase should be updated in one go followed by the inclusion of the change in .git-blame-ignore-revs

wrt deprecation - OK, let's keep it as it is and do nothing in that area for now

wrt failing ECA check - last two commits were probably done through different username (...www...) which has not signed the ECA; the first one seems to be fine (probably)... can you try to amend them?

@rmcdouga rmcdouga requested a review from lukasj April 14, 2023 13:34

@lukasj lukasj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

@lukasj lukasj added this to the 2.2 milestone Apr 14, 2023
@lukasj lukasj linked an issue Apr 14, 2023 that may be closed by this pull request
@lukasj lukasj merged commit 2669f78 into jakartaee:master Apr 30, 2023
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.

Add support for java.nio.Path

2 participants