stringify adds underscore as integer decimal separator

[zanona]

Hello 👋,
I am a bit intrigued with the fact that integers are added an underscore as a decimal separatorssuch as:

const a = {number:10000};
toml.stringify(a) // number=10_000

repro: https://codesandbox.io/s/iarnatoml-stringify-45080?file=/src/index.js

This is being used for both integers and floats

iarna-toml/stringify.js

Lines 188 to 191 in 1374bf2

	function stringifyInteger (value) {
	/* eslint-disable security/detect-unsafe-regex */
	return String(value).replace(/\B(?=(\d{3})+(?!\d))/g, '_')
	}

But it would great allowing users to disable it in case they prefer it without separators in order to improve editability?

[iarna]

I'm honestly surprised that you think it would be more editable/readable without separators. That said, using thousands separators versus ten thousand separators betrays my biases.

I'm hesitant to add configuration to the stringifier, but I have to admit that this is probably the place for it.

[scriptcoded]

I'm modifying config files for another program using this library, and I'd ideally want to stay true to their formatting, and they don't use underscores. Also, I can see a few cases where underscores might be confusing, such as with port numbers. It's a great feature for readability, but I don't think toggling it would be a bad idea.

[dsolanorush]

@iarna This package is being used in @nuxtjs/netlify-files to stringify the netlify.toml config, and it completely breaks things due to the addition of underscores for things like port numbers and dimensions. I see this has been around for 2+ years at this point, but is there any chance this could be made configurable as others have suggested?

[scriptcoded]

(whoa it sure was a while ago I was active here 😅)

@iarna I'm looking through the code to see if I could create a simple PR for adding options to the stringify function, but seeing as that part of the library consists of 25 different stringifier functions calling each other recursively, it would result in a lot of passing around of that options object.

One idea I have to solve this would be to have a Stringifier class (seeing as classes are already being used in the code base) that sets the configuration in the constructor. The public API could still remain the same, with TOML.stringify and TOML.stringify.value simply instantiating a new Stringifier and executing the appropriate method on that instance. The Stringifier class could of course optionally also be exported, though I'm not sure this would be necessary.

What are your thoughts? Keep it fully functional and pass around config? Convert to class? Maybe even skip implementing options altogether?

[dsolanorush]

I suppose a good question would be: what's the benefit of injecting the underscore as a thousands separator? Simply for improved readability? Seems like altering a value during conversion wouldn't be a desired (nor expected) effect in the first place. Certainly was not expected in my case.

That said, sounds like @scriptcoded has a reasonable suggestion with the class approach.

[scriptcoded]

@dsolanorush Yup, I agree that having no separator by default would make a lot of sense. In a perfect would I'd argue formatting should be kept upon parsing and stringification, but unfortunately that's practically impossible if you want the parsed output to be a simple object representing only the data contained. I think #42 has some relevant information regarding that topic.

All that being said, removing the separator from the formatted output could mess with other dependents, just as it does in reverse for @nuxtjs/netlify-files. It is a public API after all. To me the change seems too minor to justify a major version bump. Maybe it can be kept in mind for a future breaking release.

[dsolanorush]

@scriptcoded - All fair points. I went ahead and forked the @nuxtjs/netlify-files repo and applied a simple regex replace to the stringified TOML output. Quick and dirty, but works well for my specific use case.