Fixes #1457, int val output in TOML format, adds test #1458
Conversation
|
Deploy preview for cms-demo ready! Built with commit 7b877e0 |
|
Deploy preview for netlify-cms-www ready! Built with commit 7b877e0 |
| @@ -192,7 +192,7 @@ describe('Frontmatter', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('should stringify YAML with --- delimiters when it is explicitly set as the format without a custom delimiter', | |||
There was a problem hiding this comment.
If this file doesn't have any actual code changes, can we just clean up the diff?
src/formats/toml.js
Outdated
| @@ -4,13 +4,19 @@ import moment from 'moment'; | |||
| import AssetProxy from 'ValueObjects/AssetProxy'; | |||
| import { sortKeys } from './helpers'; | |||
|
|
|||
| // IE polyfill for Number.isInteger | |||
There was a problem hiding this comment.
We don't actually support IE, so this can be removed. 😃
There was a problem hiding this comment.
Docs refer to IE11 here. Couldn't find any other references to browser support in docs though.
...Will remove this polyfill shortly though, as I can confirm that the CMS won't load at all in IE11 right now.
There was a problem hiding this comment.
...BTW, I did find the discussion in issues here RE: browser support, with a summary at the end. 😃
There was a problem hiding this comment.
Haha, feel free to PR a removal for that if you want.
cc/ @verythorough
src/formats/toml.js
Outdated
| const outputReplacer = (key, value) => { | ||
| if (moment.isMoment(value)) { | ||
| return value.format(value._f); | ||
| } | ||
| if (value instanceof AssetProxy) { | ||
| return `${ value.path }`; | ||
| } | ||
| if (Number.isInteger(value)) { |
There was a problem hiding this comment.
I wonder if we should be checking isSafeInteger instead, so we don't accidentally output an invalid number? Or is that only a concern for computations and it shouldn't be a problem with toString()? That was the original problem that @iology noted for tomlify anyway.
There was a problem hiding this comment.
I think isInteger is sufficient... we already know we aren't dealing with a string representation of an int-like number that could overflow or lose precision. We're dealing with a straight-up number that JavaScript believes to be an int. And right, no calculations here. We're just telling tomlify not to put decimal places on the thing, which we must do by giving it a string representation of what we want rendered in the TOML file.
That was my original thought as well. I just wasn't sure if it was relevant because obviously the creator of |
|
Hi, I'm the author of "tomlify". Tomlify, which is schema-less, does not distinguish between floats and integer, and it only throw an error when the number is not finite. So if you need exact integers, you should examine the key paths and check the numbers with Number.isSafeInteger and take care of overflow and underflow in other code. |
|
OK, so @tech4him1, I understand the points made RE: |
|
Yes, thank you for your time answering my questions. 😄 |
|
@slathrop By the way, I'm sorry if I came across as argumentative -- I really appreciate you taking the time to submit this PR. I just wanted to make sure I fully understood it since there was potential for problems. Thanks! |
|
@tech4him1 Not at all, but very thoughtful of you to consider it anyway. Thanks. |
- Summary
Fixes #1457, int val output in TOML format, adds test
- Test plan
Test added and passing for int and float
- Description for the changelog
Fix int val output in TOML format file