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

Feature (v3): Add a function to set the desired line length of an Encoder #455

Closed
wants to merge 8 commits into from

Conversation

laverya
Copy link
Contributor

@laverya laverya commented Apr 23, 2019

This PR exposes a function func (e *Encoder) SetLineLength(characters int) that allows setting the line wrap length of an encoder. The interface was intended to have a very similar style to the SetIndent function func (e *Encoder) SetIndent(spaces int), implemented here.

Internally, it uses the existing function func yaml_emitter_set_width(emitter *yaml_emitter_t, width int), which was previously unused.

This relates to several issues (#387, #348, #166) and a previous PR (#329).

@bai
Copy link

bai commented May 13, 2019

👋Friendly bump: any news on when this might get shipped? 🙏

@laverya
Copy link
Contributor Author

laverya commented May 16, 2019

I'm afraid there's been no news @bai - I've emailed @niemeyer again but no replies since the 2nd (of May). Things seem to be moving slowly, but I remain hopeful that they will eventually move

@bai
Copy link

bai commented Jun 5, 2019

@laverya If I get it right, you're implementing this PR in context of Kustomize. And while this would be really helpful for other projects, Kustomize is the reason you submitted it in the first place?

Anyhow, the reason I'm asking is: do you think we could get this PR backported to Kubernetes' fork of yaml at https://github.com/kubernetes-sigs/yaml and make Kustomize use it instead?

Curious what you think, given there's not much movement going on here. ☁️ 🙄

@laverya
Copy link
Contributor Author

laverya commented Jun 6, 2019

@bai you're right that kustomize is the primary target of this (or at least MY primary target), and creating a fork of go-yaml/yaml that kubernetes-sigs/yaml can import is a plausible route to get it there. It feels like we're forking the entire chain at that point, of course 😆 (and for that matter I hadn't realized that kustomize had moved to a fork of ghodss/yaml - any idea why?)

@bai
Copy link

bai commented Jun 7, 2019

I'm not aware about exact intentions, but assume that for safety/audit reasons, given how many users use those tools with mission-critical infrastructure (to avoid things like left-pad and cryptomining npmjs fiasco).

So my thinking was that if kubernetes/kubernetes uses a wrapper/"fork" at https://github.com/kubernetes-sigs/yaml then it would naturally make sense for kustomize use it as well? 🤔

@laverya
Copy link
Contributor Author

laverya commented Jun 7, 2019

Ah, I think we talked past each other a little here - kustomize also uses kubernetes-sigs/yaml as of last month: kubernetes-sigs/kustomize#1014.

I just meant that forking go-yaml/yaml would mean that the yaml dependency chain for kustomize would look like this:

laverya/yaml (forked from go-yaml/yaml)
|
kubernetes-sigs/yaml (forked from ghodss/yaml)
|
kubernetes-sigs/kustomize

This doesn't mean that it's a bad idea, though! Just (IMO) funny.

@bai
Copy link

bai commented Jun 24, 2019

Happy Monday! 👋Friendly bumping this PR since kustomize v3.0.0-pre was tagged and it'd be great to have this included into 3.0.0.

Thoughts on doing laverya/yaml (forked from go-yaml/yaml) thing you mentioned in your last comment, since it's been 2 months since you've submitted this PR? 💭

@laverya
Copy link
Contributor Author

laverya commented Jul 9, 2019

@niemeyer seems to be back and active (which is why I merged the v3 branch again) - any chance this could get merged?

@laverya
Copy link
Contributor Author

laverya commented Jul 10, 2019

In any case, I'll be changing my repos to use https://github.com/laverya/yaml/tree/v3 for now. (gopkg.in/laverya/yaml.v3)

I'll happily switch back (and drop the maintenance burden) once this gets merged, of course.

@pupapaik
Copy link

Can we merge this please? I had to use his fork branch to fix my problem. Thanks

@laverya
Copy link
Contributor Author

laverya commented Jul 31, 2019

@pupapaik I've seen very little interest from the maintainers in actually merging PRs, I'm afraid 😦 I've still got 6 PRs open, which is really depressing. If he was willing to comment and tell me that the patches were unwanted, or that they were deficient in some way, that would be one thing - but I've yet to get a real comment on any of them.

Or at least I would assume @niemeyer is the maintainer?

EDIT: reading this later, this was inappropriate.

@niemeyer
Copy link
Contributor

niemeyer commented Jul 31, 2019

You're being pushy, Andrew. You did get comments, and you did get a whole email thread about this PR saying I was stabilizing v3 before accepting more patches. I worked quite some time fixing bugs in v3 recently, and that's not even part of my daily job. I do it because I want to make it good. Please be more patient.

@laverya
Copy link
Contributor Author

laverya commented Aug 1, 2019

I will try to be more patient. Thank you for responding!

@jmhodges
Copy link

Any word on this? I just ran into this problem.

jmhodges added a commit to jmhodges/yaml that referenced this pull request Oct 17, 2019
This adds the SetLineLength method to Encoder and changes the imports to the
jmhodges/yaml.v2 config.

The SetLineLength patch is adapated from
go-yaml#455

The README got an update for folks needing this.
jmhodges added a commit to jmhodges/yaml that referenced this pull request Oct 17, 2019
This adds the SetLineLength method to Encoder and changes the imports to the
jmhodges/yaml.v2 config.

The SetLineLength patch is adapated from
go-yaml#455

The README got an update for folks needing this.
jmhodges added a commit to jmhodges/yaml that referenced this pull request Oct 17, 2019
This adds the SetLineLength method to Encoder and changes the imports to the
jmhodges/yaml.v2 config.

The SetLineLength patch is adapated from
go-yaml#455

The README got an update for folks needing this.
@jmhodges
Copy link

For folks who need this on yaml.v2 (I needed the MapSlice API), I've published this patch on top of yaml.v2 at gopkg.in/jmhodges/yaml.v2

@laverya
Copy link
Contributor Author

laverya commented Oct 17, 2019

There's also a v3 version available here: gopkg.in/laverya/yaml.v3

...I should really merge in the latest commits from this repo. Something for tomorrow.

@bai
Copy link

bai commented Oct 17, 2019

Hopefully this PR is going to get merged soon 🤞

@nettrino
Copy link

Knowing that this is a very ugly hack - just mentioning for anyone interested that the width can also be set using reflect. I am not sure though if that breaks something else in the package.

        ....
	var buf bytes.Buffer
	enc := yaml.NewEncoder(&buf)
	v := reflect.ValueOf(enc).Elem().FieldByName("encoder").Elem()
	width := v.FieldByName("emitter").FieldByName("best_width").UnsafeAddr()
	widthPtr := (*int)(unsafe.Pointer(width))
	// don't limit the width
	*widthPtr = -1
	// encode contents
	if err := enc.Encode(node.Content[0]); err != nil {
		return err
	}
	if err := enc.Close(); err != nil {
		return err
	}

	ioutil.WriteFile(output, buf.Bytes(), permissions)

@laverya
Copy link
Contributor Author

laverya commented Oct 25, 2019

That is a horrible, unsafe and unmaintainable hack.

...I love it

@laverya laverya changed the title Add a function to set the desired line length of an Encoder Feature (v3): Add a function to set the desired line length of an Encoder Nov 5, 2019
@klautcomputing
Copy link

👋 @niemeyer Do you have an idea when this might get merged?

@niemeyer
Copy link
Contributor

I need to dedicate a moment to a few open issues here in the next few days. Will look into this as well.

@bai
Copy link

bai commented Feb 6, 2020

Apologies for being a pain in the ass with this one, any news on when this might get shipped? 🙏

@laverya laverya requested a review from niemeyer February 6, 2020 19:13
@bai
Copy link

bai commented Mar 1, 2020

1TLONGY

@ashokponkumar
Copy link

@niemeyer Friendly request to get this PR merged, when you get a chance.

@laverya
Copy link
Contributor Author

laverya commented Apr 24, 2020

If anyone's still waiting for this, the company I work for has a branch with releases over at https://github.com/replicatedhq/yaml/tree/v3

It's basically the current v3 (though with a different 'marshal yes/no/etc with quotes' implementation) plus 'marshal with json tags if those are present and yaml tags are not')

(I figured the 1 year anniversary was a good opportunity to plug that)

@ashokponkumar
Copy link

If anyone's still waiting for this, the company I work for has a branch with releases over at https://github.com/replicatedhq/yaml/tree/v3

It's basically the current v3 (though with a different 'marshal yes/no/etc with quotes' implementation) plus 'marshal with json tags if those are present and yaml tags are not')

(I figured the 1 year anniversary was a good opportunity to plug that)

Is there any issue in using your fork? It seems to fix the issue till this merge is completed.

@bai
Copy link

bai commented Apr 25, 2020

(I figured the 1 year anniversary was a good opportunity to plug that)

Happy 1 year anniversary indeed! Time flies.

@laverya
Copy link
Contributor Author

laverya commented Apr 27, 2020

Is there any issue in using your fork? It seems to fix the issue till this merge is completed.

No issues that I know of! Just go get github.com/replicatedhq/yaml/v3. (If there are issues, please let me know)

@ashokponkumar
Copy link

Is there any issue in using your fork? It seems to fix the issue till this merge is completed.

No issues that I know of! Just go get github.com/replicatedhq/yaml/v3. (If there are issues, please let me know)

Sorry that I wasn't clear. I was referring to https://github.com/laverya/yaml fork. Was trying to be as close to go-yaml/yaml fork, so that I can switch back as soon as this fix is rolled out.

@sagarkrkv
Copy link

@niemeyer bump, please consider merging this, as it is a very simple fix, our option would be to maintain a custom fork with this tiny change. We would rather avoid that if possible.

@niemeyer
Copy link
Contributor

niemeyer commented May 6, 2020

I've merged the change, also from @laverya, that disables the wraping altogether instead.

I'm tentatively closing this, in the hope that what people really want is no wrapping at all.

Let's see what happens next.

@niemeyer niemeyer closed this May 6, 2020
@laverya
Copy link
Contributor Author

laverya commented May 7, 2020

Let's hope, and that's all I think I need! I'll make PRs for some consumers of this (https://github.com/kubernetes-sigs/yaml, https://github.com/ghodss/yaml, a few downstream things) tomorrow. (Or whenever this becomes a release)

@laverya
Copy link
Contributor Author

laverya commented Nov 12, 2020

All of a sudden I'm happy I didn't actually make the PRs I was suggesting here (since the consuming projects I was using were able to go get gopkg.in/yaml.v2) - given that the 'change default wrap length to infinity' PR broke tests elsewhere when it was eventually included.

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.

None yet

9 participants