Skip to content

fix: escape leading # and ! in keys so they round-trip - #22

Open
mahirhir wants to merge 2 commits into
eemeli:mainfrom
mahirhir:fix-key-comment-marker-roundtrip
Open

fix: escape leading # and ! in keys so they round-trip#22
mahirhir wants to merge 2 commits into
eemeli:mainfrom
mahirhir:fix-key-comment-marker-roundtrip

Conversation

@mahirhir

Copy link
Copy Markdown

stringify does not escape a # or ! at the start of a key. Since those characters start a comment when they lead a line, parse reads the output back as a comment and drops the pair:

const { parse, stringify } = require('dot-properties')

parse(stringify({ '#x': 'y' })) // => {}  (the key is lost)
parse(stringify({ '!x': 'y' })) // => {}  (the key is lost)

escapeKey already escapes the separator characters (space, =, :) so they survive a round trip. The comment markers have the same problem when they lead a key, so I escape a leading # or ! there too. parse already unescapes \#/\! back to #/!, and java.util.Properties escapes these as well, so the result round-trips and stays consistent with the reference format. They are left untouched elsewhere in the key, since they are only special at the start of a line.

parse(stringify({ '#x': 'y', '!x': 'w' })) // => { '#x': 'y', '!x': 'w' }
stringify([['a#b!c', 'v']])                // => 'a#b!c = v'  (unchanged)

Added a round-trip test in tests/corner-cases.tests.js. The full suite (40 tests, 100% coverage), eslint, and prettier all pass.

A key starting with # or ! was written out unescaped, so parse() read
the line back as a comment and dropped the entry. escapeKey already
escapes the separator characters (space, =, :); escape a leading # or !
as well so such keys survive a round trip, matching java.util.Properties.

@eemeli eemeli left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks good, thank you for catching this!

See inline for a small tweak.

Comment thread lib/stringify.js Outdated
Comment on lines +16 to +17
const escapeKey = key =>
escape(key).replace(/[ =:]/g, '\\$&').replace(/^[#!]/, '\\$&')

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This should work as well?

Suggested change
const escapeKey = key =>
escape(key).replace(/[ =:]/g, '\\$&').replace(/^[#!]/, '\\$&')
const escapeKey = key => escape(key).replace(/^[#!]|[ =:]/g, '\\$&')

@mahirhir

Copy link
Copy Markdown
Author

Applied, thanks. Full suite still 100% coverage, 40/40 passing.

@eemeli eemeli left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please fix the code style, and this is good to merge.

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.

2 participants