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

Do not throw exception when Y-axis min/max properties are not set #62

Merged
merged 2 commits into from
Jul 25, 2020
Merged

Do not throw exception when Y-axis min/max properties are not set #62

merged 2 commits into from
Jul 25, 2020

Conversation

gjabouley-invn
Copy link
Contributor

@gjabouley-invn gjabouley-invn commented May 19, 2020

Jira: JENKINS-50363

Fix the GetDoubleFromString conversion exception (INFO --> FINE) when YaxisMin/Max are not set

It is a feature not to configure YaxisMinimum/Maximum. In that case, String values are defaulted to null.
As DoubleFromString convertion is called also within hasYaxisMinimum() / hasYaxisMaximum() function (strange pattern), so convertion failure is a feature and should not raise an exception with INFO level, which is bloating Jenkins logs

Silent the DoubleFromString conversion exception (INFO --> FINE)

It is a feature not to configure YaxisMinimum/Maximum. In that case, String values are defaulted to null.
As DoubleFromString convertion is called also within hasYaxisMinimum() / hasYaxisMaximum() function (strange pattern), so convertion failure is a feature and should not raise an exception with INFO level, which is bloating Jenkins logs
@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #62 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #62   +/-   ##
=========================================
  Coverage     43.38%   43.38%           
  Complexity      190      190           
=========================================
  Files            18       18           
  Lines          1263     1263           
  Branches        192      192           
=========================================
  Hits            548      548           
  Misses          651      651           
  Partials         64       64           
Impacted Files Coverage Δ Complexity Δ
src/main/java/hudson/plugins/plot/Plot.java 25.87% <0.00%> (ø) 30.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b5cee7...a40872f. Read the comment docs.

@@ -399,7 +399,7 @@ public Double getDoubleFromString(String input) {
try {
result = Double.parseDouble(input);
} catch (NumberFormatException nfe) {
LOGGER.log(Level.INFO, "Failed to parse Double value from String."
LOGGER.log(Level.FINE, "Failed to parse Double value from String."
Copy link
Contributor

Choose a reason for hiding this comment

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

@gjabouley-invn hey. have you tried actually adjusting the logic and properly verifying the input for emptiness? That was suggest in the ticket as well and looks legit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made corresponding change, it should now be aligned with your expectations

Leveraging StringUtils.IsEmty() which handles null gracefully
@gjabouley-invn gjabouley-invn changed the title JENKINS-50363 Silent the GetDoubleFromString exception when YaxisMin/Max are not set JENKINS-50363 Fix the GetDoubleFromString exception when YaxisMin/Max are not set Jul 20, 2020
@vgaidarji vgaidarji added feature New feature or improvement bug Bug fix and removed feature New feature or improvement labels Jul 25, 2020
@vgaidarji vgaidarji changed the title JENKINS-50363 Fix the GetDoubleFromString exception when YaxisMin/Max are not set Do not throw exception when Y-axis min/max properties are not set Jul 25, 2020
@vgaidarji vgaidarji merged commit 9405744 into jenkinsci:master Jul 25, 2020
@vgaidarji
Copy link
Contributor

Thanks a lot for your contribution @gjabouley-invn. This change will be included in 2.1.8 version which was just released. 🎉 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants