Skip to content

Conversation

@sankalp-khare
Copy link

@sankalp-khare sankalp-khare commented May 21, 2023

After this change what our package prints matches jq exactly. Thanks!

@sankalp-khare
Copy link
Author

sankalp-khare commented May 22, 2023

I see now that there is a printSpace, and it relies on fs.compact... is there a way to configure the package to print the space I want, without resorting to what is in this pull request?

Thanks!

@gadiener
Copy link

gadiener commented Mar 5, 2024

Would be great to have that merged

@gadiener
Copy link

gadiener commented Mar 5, 2024

@nwidger could you give a look here?

@amterp
Copy link

amterp commented Apr 22, 2025

Agree that this formatting change should be made, but this PR as-is won't work as intended. For example:

Before PR:

{
  "bar": [],
  "foo":2
}

After PR:

{
  "bar":  [],
  "foo": 2
}

Notice the []-value is already correct, but will be broken after the PR.

@amterp
Copy link

amterp commented Apr 22, 2025

I'm looking more at this issue, it seems like the library actually used to handle this case correctly, but PR #8 broke it.

Looking at that commit, it's not correct -- that added variable inputIsObjectOrArray is always false. It's initialized to false and then only ever updated as inputIsObjectOrArray = inputIsObjectOrArray && true so will always remain false. This always false variable is then used in this block:

if !frame.inField() && inputIsObjectOrArray {
    fs.printSpace(" ", false)
}

so we always fail this if condition and never print that space.

But simply undoing this commit, the issue goes away. This change must be a mistake written as-is, @nwidger thoughts on this?

I do see the issue described in #7 (which spawned #8), I am trying to find a way to fix #7 without breaking other functionality like #8 did.

amterp added a commit to amterp/jsoncolor-fork that referenced this pull request Apr 23, 2025
This test should pass, but is failing, demonstrating the issue described
in PR nwidger#9. A following commit will fix this test.
@amterp
Copy link

amterp commented Apr 23, 2025

I've raised #10 as an alternative fix here, which should avoid introducing additional issues like this #9 PR does, and while also ensuring #7 remains fixed.

Please take a look sometime @nwidger if you've got time! 🙏

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.

3 participants