Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
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 Issues describing a change to documentation. FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@theckman
Copy link
Contributor

theckman commented May 1, 2020 •
edited
Loading

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
@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label 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
Member

dmitshur commented May 5, 2020 •
edited
Loading

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
Member

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 gopherbot mentioned this issue May 6, 2020
Closed
@gopherbot
Copy link
Contributor

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
Contributor

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

@ALTree ALTree modified the milestones: Go1.14, Unplanned Jul 22, 2020
@dmitshur dmitshur mentioned this issue Sep 2, 2020
Closed
@gopherbot
Copy link
Contributor

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
Contributor

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
@ToastCheng @cagedmantis
[release-branch.go1.15] doc/go1.14: document json.Umarshal map key su…
60fdbce 
…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í <[email protected]>
Run-TryBot: Daniel Martí <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
(cherry picked from commit 47b4509)
Reviewed-on: https://go-review.googlesource.com/c/go/+/252618
Run-TryBot: Dmitri Shuralyov <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>
gopherbot pushed a commit that referenced this issue Sep 2, 2020
@ToastCheng @cagedmantis
[release-branch.go1.14] doc/go1.14: document json.Umarshal map key su…
404635c 
…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í <[email protected]>
Run-TryBot: Daniel Martí <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
(cherry picked from commit 47b4509)
Reviewed-on: https://go-review.googlesource.com/c/go/+/252617
Run-TryBot: Dmitri Shuralyov <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>
claucece pushed a commit to claucece/go that referenced this issue Oct 22, 2020
@ToastCheng @claucece
[release-branch.go1.15] doc/go1.14: document json.Umarshal map key su…
c81c7f9 
…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í <[email protected]>
Run-TryBot: Daniel Martí <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
(cherry picked from commit 47b4509)
Reviewed-on: https://go-review.googlesource.com/c/go/+/252618
Run-TryBot: Dmitri Shuralyov <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>
@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.
Assignees
No one assigned
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
8 participants
@theckman @dmitshur @ianlancetaylor @mvdan @ALTree @cuonglm @gopherbot @dylan-bourque