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

list indention style is different from pyyaml #432

Closed
thewilli opened this issue Jul 4, 2018 · 15 comments

Comments

Projects
None yet
4 participants
@thewilli
Copy link

commented Jul 4, 2018

consider the following pyyaml code (try yourself):

import yaml

print yaml.dump(
  {
    "foo": [1, 2, 3]
  },
  default_flow_style=False
)

which outputs

foo:
- 1
- 2
- 3

and the comparable js-yaml counterpart (try yourself):

const yaml = require("js-yaml");

console.log(
  yaml.safeDump({
    foo: [1, 2, 3]
  })
);

which outputs

foo:
  - 1
  - 2
  - 3

You can see that pyyaml doesn't add spaces to lists (because - counts as indention). I know both are valid, but it would be great if we could a similar output as pyyaml (especially as this project started as some kind of fork)?

Changing this line to

writeBlockSequence(state, level - 1, state.dump, compact);

changes the behaviour so it should be possible to add a configuration setting to allow both behaviours.

@puzrin

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

IMHO this request is specific and there are no big reasons to change output format or add an option. I tend to close it.

@thewilli

This comment has been minimized.

Copy link
Author

commented Jul 5, 2018

IMHO this request is specific and there are no big reasons to change output format or add an option. I tend to close it.

@puzrin After doing a small evaluation I'm pretty sure most of (many) frameworks are doing what pyyaml does. And shouldn't this library tend to stick to what appears to be standard (i.e. not emitting additional, needless whitespaces)?

@puzrin

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

  • Probably, "most of frameworks" just are just binding to libyaml. There are no standard OR recommendations about better output look.
  • "needless whitespaces" is not good argument, because yaml is about readability, not about size optimizations

You request a breaking change without valuable profit (at least, i could not understand it) - i don't like such balance.

@analytik

This comment has been minimized.

Copy link

commented Oct 22, 2018

Why would following the industry convention would be a "breaking request"? Especially if both specifications are valid.

If nothing else, this could easily be just a new flag in safeSave options.

The "profit" here is that other libraries and software (i.e. applications where you cannot easily edit the source code, e.g. kubernetes) dumps the condensed format. So when you export from one software and then edit with js-yaml, you have to constantly fight with whitespace in commits. This seems unreasonable, since it's a fairly straightforward patch.

@puzrin

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

"industry convention" is marketing bullshit bingo slang. I'd suggest to focus on technical arguments instead.

What's wrong with "other libraries and software"? If dumper produce valid yaml, why should someone fight with spaces?

@thewilli

This comment has been minimized.

Copy link
Author

commented Oct 22, 2018

"industry convention" is marketing bullshit bingo slang.

industry convention is what other devs sadly have to deal with in their day-to-day reality, may it be supporting the Internet Explorer or they way whitespace is handled...

I'd suggest to focus on technical arguments instead.

The technical argument is interoperability. Other tools make it different. It's absolutely fine to pick the current behaviour as a standard, but what's wrong with adding an optional setting, so the output can be reused with other libraries and platforms that decided to handle this case in a different way?

@puzrin

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

but what's wrong with adding an optional setting,

Adding "everything possible" make API messy very quickly. Current repo has conservative accept policy.

so the output can be reused with other libraries and platforms that decided to handle this case in a different way?

I'd like to understand, what's wrong right now? Output is valid yaml. Other yaml parser should accept it ok.

@thewilli

This comment has been minimized.

Copy link
Author

commented Oct 22, 2018

Adding "everything possible" make API messy very quickly.

And I do totally agree. But it's not that the proposal is completely messing with the output (let's configure an option to dump XML), but a different behaviour others have adopted.

I'd like to understand, what's wrong right now? Output is valid yaml. Other yaml parser should accept it ok.

My use case (and I can think of others): Some different tool chains work with a YAML content, and

  • detect changes when they are aren't any (because only whitespace has changed)
  • YAML files checked in a VCS like git will pollute the history with whitespace changes, complicating to work with actual changes (git blame will refer to whitespace- instead of content changes)
@puzrin

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Ok, finally, i understand possible use case.

IMHO, your request is not about spaces, but about making output look like libyaml. Can you guarantee, that array spaces is the only difference? It would be strange to have later additional requests about new options for quotes etc.

From the other hand, wouldn't it be better to use native libyaml wrapper for your [specific?] needs? There are should be some.

@jacob-hd

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

I require this feature.

The yaml that yaml.safeDump is outputting does not let me set the array syntax to the space-less indentation method, causing there to be incorrect git diffs for a bunch of yamls that are being parsed, modified, and re-output using the tool.

It is not too much to request that there be an option for this functionality. Perhaps if I get some time, I'll submit a PR.

@jacob-hd

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

I agree with @puzrin though, in that:

This issue should be called "Cannot specify optional indentation of array elements", this issue shouldn't have anything to do with pyyaml.

@jacob-hd

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

@thewilli In the meantime, here's this regex-based workaround for us:

        const output: string = yaml.safeDump(workflow, { lineWidth: Infinity });

        // This un-indents array elements to match the un-indented style.
        // yaml.safeDump style:
        //     abc:
        //       - 1
        //       - 2
        //       - 3
        // un-indented style:
        //     abc:
        //     - 1
        //     - 2
        //     - 3
        const fixedOutput: string = output.replace(/(\s+)(\s\s\-)(.*\n)/g, '$1-$3');

EDIT: Nevermind, this doesn't work because it doesn't un-indent the object properties in object arrays.

@jacob-hd

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

I have a PR open here: #461

puzrin added a commit that referenced this issue Jan 4, 2019

Add "noArrayIndent" option (#461)
Addresses issue #432 by adding a `noArrayIndent` option to optionally not add an extra level of indentation to array elements.

When `noArrayIndent` option is set to `false` (or not provided), output is:
```
array:
  - a
  - b
  - c
```

When `noArrayIndent` option is set to `true`, output is:
```
array:
- a
- b
- c
```

This helps avoid diffs when parsing, modifying, and generating valid yaml that does *not* use extra indentation for arrays.
@jacob-hd

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

@thewilli @puzrin Now that #461 has landed, it's possible that we can close this issue?

@puzrin

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

Released 3.12.1 with new option, see #461

@jacob-hd thanks for PR!

@puzrin puzrin closed this Jan 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.