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

Revert "Set the default line length to infinity (-1) (#571)" #670

Closed
wants to merge 1 commit into from

Conversation

@dims
Copy link

@dims dims commented Nov 12, 2020

This reverts commit 0b1645d.

Hi,
At kubernetes, we are having a hard time with this change as it changes default behavior with no recourse to setting a custom width (to what it was before). Please see the following scenarios we hit recently:

Signed-off-by: Davanum Srinivas davanum@gmail.com

This reverts commit 0b1645d.

Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@dims
Copy link
Author

@dims dims commented Nov 12, 2020

@jjshoe
Copy link

@jjshoe jjshoe commented Nov 12, 2020

Why revert? Why not make it configurable, if that's what you need?

@jjshoe
Copy link

@jjshoe jjshoe commented Nov 12, 2020

If this is merged it will break all the code out there that's had work arounds removed.

@jjshoe
Copy link

@jjshoe jjshoe commented Nov 12, 2020

I would also suggest a test be added for this.

@dims
Copy link
Author

@dims dims commented Nov 12, 2020

@jjshoe happy to the necessary work either way (add a test or change the patch to make it configurable). If you prefer the configurable path, what would that look like? (i.e, how can we specify the setting?)

@laverya
Copy link
Contributor

@laverya laverya commented Nov 12, 2020

It'd look something like this, IMO: #455

(note that that PR was for go-yaml.v3, not v2 - the code is pretty much the same though)

@bobbypage
Copy link

@bobbypage bobbypage commented Nov 12, 2020

+1, #571 was a breaking change and is now causing issues for updating vendored dependencies in github.com/kubernetes/kubernetes due to tests expecting the old line length.

It would be great to either revert this breaking change or make it configurable.

@jjshoe
Copy link

@jjshoe jjshoe commented Nov 12, 2020

@bobbypage just as quick to update the tests over in kubernetes, no?

@bobbypage
Copy link

@bobbypage bobbypage commented Nov 12, 2020

I think there may be further reaching issues across kubernetes and it's consumers due to this change see kubernetes/kubernetes#95571 (comment). @liggitt may have some more context there.

@liggitt
Copy link
Contributor

@liggitt liggitt commented Nov 12, 2020

making it configurable seems useful, but preserving the default behavior of yaml.v2 prior to 0b1645d is what I would expect

@jjshoe
Copy link

@jjshoe jjshoe commented Nov 12, 2020

Either way, if v2 is deprecated, and you move up to v3, you'll need to handle the change?

@liggitt
Copy link
Contributor

@liggitt liggitt commented Nov 12, 2020

Either way, if v2 is deprecated, and you move up to v3, you'll need to handle the change?

I would expect yaml.v3 to give configuration options to allow compatible output for yaml.v2 users, but yes, changing the default in yaml.v3 would be reasonable

@niemeyer
Copy link
Contributor

@niemeyer niemeyer commented Nov 13, 2020

It was clearly a mistake to accept the formatting change in v2. The current situation is unfortunate, though, because if we remove the change we'll break some people twice to avoid breaking some people once. Given the size of Kubernetes both in terms of code and community, I'm willing to compromise on the side of reverting the change made to v2, but to reduce the pain of those that already made the migration towards the larger change, I'll also add an API that will enable the new behavior so that people can simply add a line to their code and not have to reformat everything again. I'll spend some time on this issue later today.

@liggitt
Copy link
Contributor

@liggitt liggitt commented Nov 13, 2020

to reduce the pain of those that already made the migration towards the larger change, I'll also add an API that will enable the new behavior so that people can simply add a line to their code and not have to reformat everything again

That seems like a good outcome. Thanks for looking at it.

@dims
Copy link
Author

@dims dims commented Nov 13, 2020

@niemeyer sounds great! thanks for doing this. please let me know if i should close this PR.

niemeyer added a commit that referenced this pull request Nov 17, 2020
It was clearly a mistake to accept the default formatting change in v2,
and now there's no ideal choice. Either we revert the change and break
some projects twice, or we keep the change and break some projects once.
Given the report comes from Kubernetes, which has a relevant community
and code size, we'll revert it. At the same time, to simplify the life
of those that already started migrating towards the v3 behavior, a new
FutureLineWrap function is being introduced to trivially preserve the
new behavior where desired.

The v3 branch is not affected by this, and will retain the default
non-wrapping behavior. It will also be changed soon to support per
arbitrary line-wrapping for individual encoding operations.

Thanks to everyone who offered code and ideas, and apologies for
the trouble.
@niemeyer
Copy link
Contributor

@niemeyer niemeyer commented Nov 17, 2020

There it is: 7649d45

Please let me know if this looks reasonable and I'll add a version tag soon.

@liggitt
Copy link
Contributor

@liggitt liggitt commented Nov 17, 2020

There it is: 7649d45

Please let me know if this looks reasonable and I'll add a version tag soon.

looks good to me, thanks

@liggitt
Copy link
Contributor

@liggitt liggitt commented Nov 17, 2020

looks good to me, thanks

Consumers still run the risk of poorly behaved things in their dependency tree toggling global options, but that's a general issue not specific to yaml or this option. The only thing I can think of to add would maybe be a comment indicating that global option should generally be set by the process owner (e.g. the main() implementer), not by a library

@dims
Copy link
Author

@dims dims commented Nov 17, 2020

@niemeyer thanks a ton. +1 to "add a version tag"

@dims
Copy link
Author

@dims dims commented Nov 17, 2020

i'll close this out as well.

@niemeyer
Copy link
Contributor

@niemeyer niemeyer commented Nov 25, 2020

Thanks for the feedback. v2.4.0 is now tagged.

@liggitt That's a good point, but there's too much nuance to be able to easily provide guidance. Most proper software is split into independent packages internally, which may end up being the right place for that in some cases. Let's see what happens next. Hopefully v3 can soon take over and that issue will be history. I need to focus on the few open issues there and get it ready for a release.

edoger added a commit to edoger/zkits-configurator that referenced this pull request Nov 26, 2020
anthonyfok added a commit to spf13/cobra that referenced this pull request Feb 1, 2021
The previous breaking change in yaml.v2 v2.3.0 has been reverted,
see go-yaml/yaml#670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants