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

doc: include fix for #34437 in Go 1.14 release notes #38801

Closed
theckman opened this issue May 1, 2020 · 12 comments
Closed

doc: include fix for #34437 in Go 1.14 release notes #38801

theckman opened this issue May 1, 2020 · 12 comments
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@theckman
Copy link
Contributor

theckman commented May 1, 2020

Hello,

I wanted to reach out and see whether there is an interest in the fix for #34437 being included in the Go release notes. Someone I know was finally working through upgrading to Go 1.14 and ran in to some peculiar issues with their JSON being unmarshaled differently. Here is that commit on our side:

It took them some time to figure it out, while it having been mentioned in the release notes could have saved them some time (as they looked there first). I understand we can't document all fixes without the release notes becoming silly, but wondered whether that change may meet the bar of inclusion.

Cheers!

@theckman theckman changed the title release notes: include fix for #34437 in release notes 1.14 release notes: include fix for #34437 in release notes May 1, 2020
@theckman theckman changed the title 1.14 release notes: include fix for #34437 in release notes 1.14 release notes: include fix for #34437 May 1, 2020
@dylan-bourque
Copy link

I am @theckman 's "someone I know" and this issue cost me several hours today. We had a test that was failing for me locally using Go 1.14.2 but passing for others and in all of our CI pipelines. After quite a bit of investigation and experimentation I was able to determine that the failure was specific to Go 1.14+.

Admittedly, this particular test was using bad/invalid test data (an invalid enum value in a JSON test data file for an enum that implements encoding/json.TextUnmarshaler). That being said, I checked the 1.14 release notes first and was not able to find any mention of this change in behavior so I (and several coworkers) continued to dig for clues about why this test was failing now.

@mvdan
Copy link
Member

mvdan commented May 2, 2020

A somewhat similar issue was #37308, though it was filed before the final release and the notes were published.

I understand we can't document all fixes without the release notes becoming silly, but wondered whether that change may meet the bar of inclusion.

Like in the other case, this is a bugfix to make the behavior follow the docs more closely. I don't oppose adding this to the release notes, but I'm not sure if it's normal for them to be updated this late, or if we can consider it important enough given that it hasn't raised any other flags since the pre-releases or final release over two months ago.

I'd definitely be in favor of tweaking the release notes if this discussion took place during the beta or release candidate, though.

@theckman
Copy link
Contributor Author

theckman commented May 4, 2020

@mvdan people will be upgrading to 1.14+ for a long while still. What harm is there in adding this now?

@ianlancetaylor
Copy link
Contributor

I don't know anything about this particular issue but I think it's fine to update the release notes after the release has gone out. We've done it before.

@dmitshur dmitshur changed the title 1.14 release notes: include fix for #34437 doc: include fix for #34437 in Go 1.14 release notes May 5, 2020
@dmitshur dmitshur added this to the Go1.14 milestone May 5, 2020
@dmitshur dmitshur added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 5, 2020
@dmitshur
Copy link
Contributor

dmitshur commented May 5, 2020

As Ian mentioned, it shouldn't be an issue to update the release notes after the release. We don't want to do it too often and too much after a release as that adds noise, but there haven't been too many request to add missing things to Go 1.14 release notes as far as I know.

Changes to the behavior of the encoding/json package seem like something that's helpful to include to me, and there will be more people updating to Go 1.14 over time, so I'm in favor of doing it if it's deemed to be helpful by the package owners.

@mvdan, you're one of encoding/json owners, so if you're okay with it, feel free to move this issue to NeedsFix state.

/cc @cuonglm who worked on the CL, and @dsnet @bradfitz per owners.

@cuonglm
Copy link
Member

cuonglm commented May 6, 2020

👍 for updating 1.14.x release note

@mvdan
Copy link
Member

mvdan commented May 6, 2020

As long as the release team is happy with updating the release notes at this point, I don't have a problem with the change. That's what I tried to say in my previous comment.

@dmitshur dmitshur added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 6, 2020
@dmitshur
Copy link
Contributor

dmitshur commented May 6, 2020

@gopherbot Please open an issue to backport this to Go 1.14 when the fix is available. This is a documentation fix.

@gopherbot
Copy link

Backport issue(s) opened: #38904 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link

Change https://golang.org/cl/237857 mentions this issue: doc/go1.14: document that TextUnmarshaler supports map keys with string underlying types

@gopherbot
Copy link

Change https://golang.org/cl/252617 mentions this issue: [release-branch.go1.14] doc/go1.14: document json.Umarshal map key support of TextUnmarshaler

@gopherbot
Copy link

Change https://golang.org/cl/252618 mentions this issue: [release-branch.go1.15] doc/go1.14: document json.Umarshal map key support of TextUnmarshaler

gopherbot pushed a commit that referenced this issue Sep 2, 2020
…pport of TextUnmarshaler

Document that json.Unmarshal supports map keys whose underlying
types implement encoding.TextUnmarshaler.

Updates #38801.
Fixes #41178.

Change-Id: Icb9414e9067517531ba0da910bd4a2bb3daace65
Reviewed-on: https://go-review.googlesource.com/c/go/+/237857
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 47b4509)
Reviewed-on: https://go-review.googlesource.com/c/go/+/252618
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
gopherbot pushed a commit that referenced this issue Sep 2, 2020
…pport of TextUnmarshaler

Document that json.Unmarshal supports map keys whose underlying
types implement encoding.TextUnmarshaler.

Updates #38801.
Fixes #38904.

Change-Id: Icb9414e9067517531ba0da910bd4a2bb3daace65
Reviewed-on: https://go-review.googlesource.com/c/go/+/237857
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 47b4509)
Reviewed-on: https://go-review.googlesource.com/c/go/+/252617
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
…pport of TextUnmarshaler

Document that json.Unmarshal supports map keys whose underlying
types implement encoding.TextUnmarshaler.

Updates golang#38801.
Fixes golang#41178.

Change-Id: Icb9414e9067517531ba0da910bd4a2bb3daace65
Reviewed-on: https://go-review.googlesource.com/c/go/+/237857
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
(cherry picked from commit 47b4509)
Reviewed-on: https://go-review.googlesource.com/c/go/+/252618
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
@golang golang locked and limited conversation to collaborators Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

8 participants