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

feat: support parse and stringify with comments #247

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

lheberlie
Copy link

@lheberlie lheberlie commented Mar 13, 2024

support option to save comments when stringifying

#32
#83

image

parse(data, { preserveComments: true } )
parse(data, { commentsKey: ‘internalComments’, preserveComments: true } )
stringify(data, { preserveComments: true })
stringify(data, { commentsKey: ‘internalComments’, preserveComments: true } )

opt in to save comments when parsing and stringify
save comments to internal object key upon parsing,
add the option to save when stringifying data
store comments internally upon parse and stringify to avoid conflicts
when parsing and stringify multiple documents
use platform eol
include tests in default /test/foo.js
Test option for reading (parse) and saving (stringify) comments in
configuration file.

Test option for reading (parse) and saving (stringify) comments in
configuration file using a custom commentsKey.
Option for reading (parse) and saving (stringify) comments in
configuration file.
@lheberlie lheberlie requested a review from a team as a code owner March 13, 2024 23:39
lib/ini.js Fixed Show resolved Hide resolved
lib/ini.js Fixed Show fixed Hide fixed
'{1,}' can be simplified to '+'
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data
https://codeql.github.com/codeql-query-help/javascript/js-polynomial-redos/
lib/ini.js Outdated Show resolved Hide resolved
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data
https://codeql.github.com/codeql-query-help/javascript/js-polynomial-redos/
lib/ini.js Outdated Show resolved Hide resolved
If line starts with comment character ; or #
match line string and push line into line comment array
lib/ini.js Show resolved Hide resolved
lib/ini.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

Looking good so far, I'll defer to @lukekarrys for the final review since they started it in #246. This probably will not be looked at till next week at the earliest, just a heads up.

@harshita2405
Copy link

Hi Team and @lukekarrys, any update on this. I also have a similar use case as the above. Do we know when can we expect this to go in ?

if (preserveComments) {
out.comments = commentsDictionary
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this will work because the comments key on the returned object could have already been set in the ini text being decoded.

Here's an example:

> require('ini').decode('comments = this is in my ini file')
{ comments: 'this is in my ini file' }

> require('ini').decode('comments = this is in my ini file',{preserveComments:true})
{ comments: {} }

I think the best strategy here would be to follow what json-parse-even-better-errors does and store this data as a symbol on the returned object which can then be read back when encoding. Here's a reference to that code: https://github.com/npm/json-parse-even-better-errors/blob/9355df83b4ce4567711823fc1b40fe63dbeebba5/lib/index.js#L106-L108

Another option would be new methods like decodeWithComments which could return a tuple of [data, comments] which could then be passed back to a new encodeWithComments. But I think the symbols would be a better API if that will work.

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

Successfully merging this pull request may close these issues.

None yet

4 participants