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

Amends #3211: Restore Source compatibility of hudson.util.TextFile#lines() #3340

Merged
merged 2 commits into from Mar 11, 2018

Conversation

3 participants
@oleg-nenashev
Member

oleg-nenashev commented Mar 10, 2018

In #3211 @dtrebbien retained the binary comatibility, but not a source compatibility. It would cause failures in tools like PCT for plugins using the method.

Proposed changelog entries

  • RFE: Developer: Introduce the hudson.util.TextFile#linesStream() method for file stream processing with proper error propagation

Submitter checklist

N/A

Desired reviewers

@daniel-beck @jenkinsci/code-reviewers

@ghost

This comment has been minimized.

ghost commented Mar 10, 2018

Is the problem the fact that with #3211, TextFile.lines() now throws IOException?

I believe that the restored, deprecated lines() can be implemented with:

@Deprecated
public @Nonnull Iterable<String> lines() {
    try {
        return readLines();
    } catch (IOException ex) {
        throw new RuntimeException(ex);
    }
}

Or, if the Iterable should allow multiple iterators to be created:

@Deprecated
public @Nonnull Iterable<String> lines() {
    return new Iterable<String>() {
        @Override
        public Iterator<String> iterator() {
            try {
                return readLines().iterator();
            } catch (IOException ex) {
                throw new RuntimeException(ex);
            }
        }
    };
}

.. because LinesStream already has code to close the underlying stream upon reaching the end:

https://github.com/jenkinsci/jenkins/blob/145c606e83498ca39430d7d3969c50418b26b925/core/src/main/java/jenkins/util/io/LinesStream.java#L95..L104

Also, on naming, I think that readLines() is a little confusing because creating a LinesStream does not read any lines at that point. Perhaps like was done in #3211, it could be named lines2()? Or, how about linesStream()?

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Mar 10, 2018

@dtrebbien thanks for the quick review! Addressed the comments

@ghost

This comment has been minimized.

ghost commented Mar 10, 2018

Looks good to me

@oleg-nenashev oleg-nenashev requested a review from daniel-beck Mar 10, 2018

@oleg-nenashev oleg-nenashev merged commit 7ef9e65 into jenkinsci:master Mar 11, 2018

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
@jglick

This comment has been minimized.

Member

jglick commented Mar 12, 2018

It would cause failures in tools like PCT for plugins using the method.

Huh? PCT compiles code & tests against the original core, then runs tests against the new core. So only binary compatibility matters.

@daniel-beck daniel-beck referenced this pull request Mar 14, 2018

Merged

[JEP-202] Extending VirtualFile #3302

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