jdk icon indicating copy to clipboard operation
jdk copied to clipboard

8305457: Implement java.io.IO

Open pavelrappo opened this issue 9 months ago • 9 comments

Please review this PR which introduces the java.io.IO top-level class and three methods to java.io.Console for Implicitly Declared Classes and Instance Main Methods (Third Preview).

This PR has been obtained as git merge --squash of a now obsolete draft PR.


Progress

  • [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] Change requires CSR request JDK-8331610 to be approved

Issues

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19112/head:pull/19112
$ git checkout pull/19112

Update a local copy of the PR:
$ git checkout pull/19112
$ git pull https://git.openjdk.org/jdk.git pull/19112/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19112

View PR using the GUI difftool:
$ git pr show -t 19112

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19112.diff

Webrev

Link to Webrev Comment

pavelrappo avatar May 06 '24 21:05 pavelrappo

:wave: Welcome back prappo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar May 06 '24 21:05 bridgekeeper[bot]

@pavelrappo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8305457: Implement java.io.IO

Reviewed-by: naoto, smarks, jpai, jlahoda

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

  • 6a35311468222f9335b43d548df2ecb80746b389: 8241550: [macOS] SSLSocketImpl/ReuseAddr.java failed due to "BindException: Address already in use"
  • f16265d69b09640b972b7494ad57158dbdc426bb: 8332226: "Invalid package name:" from source launcher
  • 5a2ba952b120394d7cc0d0890619780c1c27a078: 8325841: Remove unused references to vmSymbols.hpp

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk[bot] avatar May 06 '24 21:05 openjdk[bot]

@pavelrappo The following labels will be automatically applied to this pull request:

  • core-libs
  • kulla

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

openjdk[bot] avatar May 06 '24 21:05 openjdk[bot]

Otherwise looks good.

forax avatar May 07 '24 05:05 forax

Mailing list message from Olexandr Rotan on core-libs-dev:

Has it been considered to add a readKey() method to IO class? In my experience, it is pretty commonly used by beginners when they write things like console games (snake, catcher game etc.). I am aware there is System.in.read() method, but since we decided to promote some methods to separate methods in IO class, maybe readKey() or simple read() should also be considered?

On Wed, May 8, 2024 at 4:16?PM Pavel Rappo <prappo at openjdk.org> wrote:

-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240508/28c90cb6/attachment-0001.htm>

mlbridge[bot] avatar May 08 '24 17:05 mlbridge[bot]

Mailing list message from Pavel Rappo on core-libs-dev:

I don't remember it being considered, but I think it falls outside the scope of JEP 477 (emphasis mine):

We achieve this effect by declaring a new top-level class in the java.io package named, simply, IO. It declares the above three static methods for **textual** I/O with the console, and nothing else.

java.io.IO is closely related to java.io.Console, which provides access to a terminal in a more or less canonical mode to read and write _strings_. While it might be useful for Console to separately provide access to a terminal in the raw mode, I'm not sure how often developers, let alone on-ramp developers, need it. In my experience, programming games, such as those you mentioned, is better introduced with GUI, where they have java.awt.event.KeyListener.

-Pavel

mlbridge[bot] avatar May 08 '24 17:05 mlbridge[bot]

(Minor). at least within the context of this PR, tags like @param and @throws are stylistically inconsistent as to whether they begin with a capital letter and end with a period. (I have not checked for local consistency within the affected files.)

I know, thanks. That was on purpose to mimic the prevailing style of Console, just like you assumed in parentheses. Note, like-named methods in IO are styled differently.

pavelrappo avatar May 09 '24 15:05 pavelrappo

Latest changes look good.

stuart-marks avatar May 10 '24 04:05 stuart-marks

@pavelrappo this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8305457-Implement-java.io.IO
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar May 21 '24 02:05 openjdk[bot]

/integrate

pavelrappo avatar May 24 '24 13:05 pavelrappo

Going to push as commit c099f14f07260713229cffbe7d23aa8305415a67. Since your change was applied there have been 3 commits pushed to the master branch:

  • 6a35311468222f9335b43d548df2ecb80746b389: 8241550: [macOS] SSLSocketImpl/ReuseAddr.java failed due to "BindException: Address already in use"
  • f16265d69b09640b972b7494ad57158dbdc426bb: 8332226: "Invalid package name:" from source launcher
  • 5a2ba952b120394d7cc0d0890619780c1c27a078: 8325841: Remove unused references to vmSymbols.hpp

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar May 24 '24 13:05 openjdk[bot]

@pavelrappo Pushed as commit c099f14f07260713229cffbe7d23aa8305415a67.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk[bot] avatar May 24 '24 13:05 openjdk[bot]