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

Wrong quote position writing condensed flow #526

Closed
GabrieleC opened this issue Nov 14, 2019 · 8 comments
Closed

Wrong quote position writing condensed flow #526

GabrieleC opened this issue Nov 14, 2019 · 8 comments

Comments

@GabrieleC
Copy link
Contributor

This document:

key1: val1
key2: val2

dumped with options { flowLevel: 0, condenseFlow: true } produces output with misplaced quote:

{"key1":val1", key2":val2}
@puzrin
Copy link
Member

puzrin commented Nov 14, 2019

Dupe of #412

@puzrin puzrin closed this as completed Nov 14, 2019
@GabrieleC
Copy link
Contributor Author

I made a simple bugfix for this, should i discard it or make a PR?

@puzrin
Copy link
Member

puzrin commented Nov 14, 2019

I guess those options are mutually exclusive. condenseFlow was added for concrete case - URL encoding, and i would not like to spread it wide without need. If you wish this to be changed, this should be discussed first.

@GabrieleC
Copy link
Contributor Author

When using condenseFlow: true:

  • if flowLevel == -1 (default value) all collections will be write as blocks, so condenseFlow: true will be useless.
  • if flowLevel >= 0 there will be the bug.

So either "condenseFlow" is useless or it's broken.

There's something I'm misunderstanding?

@puzrin
Copy link
Member

puzrin commented Nov 14, 2019

We do nod add features "just for fun". As i said, this option was introduced for concrete use case, and not expected to be used anyhow else. If you wish this to be changed - describe your use case first, before any coding.

@GabrieleC
Copy link
Contributor Author

The use case I'm referring is URL encoding. condenseFlow is intended to be used with flowLevel = 0, otherwise it would be useless. An example is shown in the original issue: #346.

The bug is in dumper.js, line 572, where comma is appended after the quote instead of before.

I understand this library isn't intended to be used as minifier, so if this fix isn't well accepted I will not go further.

@puzrin puzrin reopened this Nov 14, 2019
@puzrin
Copy link
Member

puzrin commented Nov 14, 2019

Probably i missunderstood you. Take a look at https://github.com/nodeca/js-yaml/pull/358/files, there are some tests. If something is missed and works not as intended in original PR, fix will be accepted.

Just make sure that's really a bug, because this option should be actively used by PR authors and they did not reported problem before.

@GabrieleC
Copy link
Contributor Author

GabrieleC commented Nov 14, 2019

The original test doesn't throw error because it uses a collection with only one item, so no comma is written.

I think this bug wasn't included in the original PR (#358), because code is much different from the current version. Maybe PR authors never upgraded to a newer version with the bug.

I made a PR including the fix and an improvement to the original test to intercept the bug: #527

puzrin pushed a commit that referenced this issue Nov 15, 2019
Fix issue #526: wrong quote position writing condensed flow
@puzrin puzrin closed this as completed Nov 15, 2019
mikebryant pushed a commit to mikebryant/renovate-test-changelogjson that referenced this issue May 27, 2020
#### [\`v3.14.0\`](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md#3140---2020-05-22)

##### Changed

-   Support `safe/loadAll(input, options)` variant of call.
-   CI: drop outdated nodejs versions.
-   Dev deps bump.

##### Fixed

-   Quote `=` in plain scalars [#519](nodeca/js-yaml#519).
-   Check the node type for `!<?>` tag in case user manually specifies it.
-   Verify that there are no null-bytes in input.
-   Fix wrong quote position when writing condensed flow, [#526](nodeca/js-yaml#526).
renovate bot pushed a commit to mikebryant/renovate-test-changelogjson that referenced this issue Jun 17, 2020
##### [\`v3.14.0\`](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md#3140---2020-05-22)

##### Changed

-   Support `safe/loadAll(input, options)` variant of call.
-   CI: drop outdated nodejs versions.
-   Dev deps bump.

##### Fixed

-   Quote `=` in plain scalars [#519](nodeca/js-yaml#519).
-   Check the node type for `!<?>` tag in case user manually specifies it.
-   Verify that there are no null-bytes in input.
-   Fix wrong quote position when writing condensed flow, [#526](nodeca/js-yaml#526).
This was referenced Mar 16, 2021
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

No branches or pull requests

2 participants