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: stringify with comments #246

Closed
wants to merge 3 commits into from

Conversation

lheberlie
Copy link

@lheberlie lheberlie commented Mar 12, 2024

support option to save comments when stringifying

#32
#83

image

stringify(data, { comments: true })
save comments to dictionary object upon parsing,
add the option to save when stringifying data
@lheberlie lheberlie requested a review from a team as a code owner March 12, 2024 21:28
Copy link
Contributor

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I gave it a quick review on mobile. Let me know what you think of the suggestions.

* Whether to save the comments.
*/

comments: false ,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
comments: false ,
comments: false,

@@ -130,6 +145,10 @@ const decode = (str, opt = {}) => {
continue
}
p = out[section] = out[section] || Object.create(null)
if (lineCommentArray.length > 0) {
commentsDictionary[section] = lineCommentArray.join('\n') + '\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should use platform eol also.

@@ -165,6 +184,10 @@ const decode = (str, opt = {}) => {
p[key].push(value)
} else {
p[key] = value
if (lineCommentArray.length > 0) {
commentsDictionary[key] = lineCommentArray.join('\n') + '\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

this one too

@@ -1,4 +1,5 @@
const { hasOwnProperty } = Object.prototype
const 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 to have as a top level dictionary shared across invocations.

If i'm reading it right, if two documents are parsed and they have keys that collide, the comments could end up in the wrong document when stringified in a different order.

// section |key = value
const re = /^\[([^\]]*)\]\s*$|^([^=]+)(=(.*))?$/i
const lines = str.split(/[\r\n]+/g)
const duplicates = {}

for (const line of lines) {
if (!line || line.match(/^\s*[;#]/) || line.match(/^\s*$/)) {
if (line && line.match(commentsRegEx)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This if block does not need to be nested within its parent. I think it would be better to have a block that checks for empty lines and another that checks for comments.

const s = i.stringify(d, { comments: true })
t.matchSnapshot(s)
t.end()
})
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 test case needs to be its own file. It can be tested within the current foo test file.

@lheberlie
Copy link
Author

Closing in favor of #247
@lukekarrys - hopefully I have addressed your suggestions in this new pull request.

@lheberlie lheberlie closed this Mar 13, 2024
@lheberlie lheberlie deleted the feat-stringify-with-comments branch March 14, 2024 13:06
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

2 participants