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

Export Float and Double as String #1561

Merged
merged 9 commits into from Feb 2, 2021

Conversation

schottsfired
Copy link
Contributor

Possible solution for #1560.

Setting raw to false for Float and Double types tells ConfigurationAsCode#toYaml to export using the DOUBLE_QUOTED style instead of the current PLAIN style:

} else if (scalar.isRaw()) {
style = PLAIN;
} else {
style = DOUBLE_QUOTED;
}

@schottsfired
Copy link
Contributor Author

schottsfired commented Jan 27, 2021

failed with:

Error:  Failures: 
Error:    DataBoundConfiguratorTest.exportYaml:99 expected:<[!!float "12.34"]> but was:<[12.34]>

I'm not sure if !!float "12.34" is importable - will check.

Copy link

@kerogers-cloudbees kerogers-cloudbees left a comment

Choose a reason for hiding this comment

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

seems like that will work.

@schottsfired
Copy link
Contributor Author

I'm not sure if !!float "12.34" is importable

Confirmed, values in this format are not importable.

Interestingly you can run the following snippet through http://yamllint.com and the result is "valid YAML" with !!float automatically removed:

test: !!float "1.23"

I wonder if !!float can be removed before it's printed on the "View Configuration" screen? 🤔

@schottsfired
Copy link
Contributor Author

Error:  Failures: 
Error:    DataBoundConfiguratorTest.exportYaml:99 expected:<["12.34"]> but was:<[12.34]>

Looks good!

@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #1561 (a1ad6da) into master (f9d526b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1561      +/-   ##
============================================
+ Coverage     81.01%   81.04%   +0.03%     
  Complexity      810      810              
============================================
  Files            66       66              
  Lines          2370     2369       -1     
  Branches        328      328              
============================================
  Hits           1920     1920              
  Misses          349      349              
+ Partials        101      100       -1     
Impacted Files Coverage Δ Complexity Δ
...a/io/jenkins/plugins/casc/ConfigurationAsCode.java 79.88% <ø> (-0.06%) 102.00 <0.00> (-1.00)
...ain/java/io/jenkins/plugins/casc/model/Scalar.java 88.67% <100.00%> (+1.88%) 23.00 <0.00> (+1.00)

@schottsfired schottsfired marked this pull request as ready for review January 27, 2021 21:14
@schottsfired schottsfired requested a review from a team as a code owner January 27, 2021 21:14
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

LGTM

@timja timja added the bug label Jan 27, 2021
@timja
Copy link
Member

timja commented Jan 27, 2021

Before we merge from what I remember this did work standalone with snake yaml and we couldn’t figure out why it didn’t work in jcasc, or is it a snake yaml limitation?

we can ship this workaround but would be interested to know more of why this is needed

Comment on lines 82 to +83
foo.setDbl(12.34);
foo.setFlt(1f); // whole numbers are exported as "<number>.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also true for doubles

@schottsfired
Copy link
Contributor Author

Hi @timja , I understand how this PR masks a potential bug in snakeyaml, but I didn't find any evidence of said bug and hit a dead end while debugging, as described in #1560.

We've heard multiple reports that exported CasC YAML isn't imported properly, and I don't think it's immediately obvious to people that decimal numbers must be quoted for the import to work. So this PR takes care of it automatically, sticking to the rule that what is exported is also importable.

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

Successfully merging this pull request may close these issues.

None yet

4 participants