Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Dec 28, 2017

These changes were suggested by Extra Hints for NetBeans IDE:
http://plugins.netbeans.org/plugin/73447/

This is somewhat related to JENKINS-42934.

Proposed changelog entries

None required

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

These changes were suggested by Extra Hints for NetBeans IDE:
http://plugins.netbeans.org/plugin/73447/
@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Dec 29, 2017
StringBuilder str = new StringBuilder((int)logfile.length());

try (BufferedReader r = new BufferedReader(new InputStreamReader(Files.newInputStream(logfile.toPath()), charset))) {
try (BufferedReader r = Files.newBufferedReader(logfile.toPath(), charset)) {
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to use Util.fileToPath to avoid Runtime exceptions. But it can be done in a separate PR

Copy link
Author

Choose a reason for hiding this comment

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

I created PR #3219 to use fileToPath().

File queueFile = getQueueFile();
if (queueFile.exists()) {
try (BufferedReader in = new BufferedReader(new InputStreamReader(Files.newInputStream(queueFile.toPath())))) {
try (BufferedReader in = Files.newBufferedReader(queueFile.toPath(), Charset.defaultCharset())) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we're not using UTF-8 here.

Copy link
Member

Choose a reason for hiding this comment

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

dunno. I am fine with fixing it in a separate PR. So far I think that all admins running Jenkins with non-UTF-8 just like to live dangerously, so I just wish them good luck

Copy link
Author

Choose a reason for hiding this comment

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

I will submit a PR to make this change. Because it will conflict with PR #3219, I will wait until PR #3219 is merged before submitting a PR to change the charset.

Copy link
Author

Choose a reason for hiding this comment

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

I created PR #3224 to switch to UTF-8.

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Jan 8, 2018
@oleg-nenashev oleg-nenashev merged commit d6ff764 into jenkinsci:master Jan 8, 2018
@oleg-nenashev
Copy link
Member

Thanks @dtrebbien !

@ghost ghost mentioned this pull request Jan 8, 2018
4 tasks
oleg-nenashev pushed a commit that referenced this pull request Jan 9, 2018
* Use fileToPath()

This is a follow-up to PR 3210 on GitHub. See:
#3210 (comment)

* Switch two File.toPath() calls to Util.fileToPath()

* Delete an unused import
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants