Update to work with released log/slog #1320

jcmuller · 2023-08-14T20:51:35Z

This is based on #1318

CLAassistant · 2023-08-14T20:51:41Z

All committers have signed the CLA.

codecov · 2023-08-15T18:08:39Z

Codecov Report

Merging #1320 (4d5fbfb) into master (d2aeb27) will decrease coverage by 0.32%.
The diff coverage is 86.59%.

@@            Coverage Diff             @@
##           master    #1320      +/-   ##
==========================================
- Coverage   97.77%   97.45%   -0.32%     
==========================================
  Files          51       52       +1     
  Lines        3366     3463      +97     
==========================================
+ Hits         3291     3375      +84     
- Misses         65       77      +12     
- Partials       10       11       +1

Files Changed	Coverage Δ
exp/zapslog/slog_pre_go121.go	`86.59% <ø> (ø)`
exp/zapslog/slog_go121.go	`86.59% <86.59%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

abhinav

Thanks for this change! I've left some suggestions.
Would you mind also updating the zapslog/doc.go (or creating one if it doesn't exist)
to indicate that it'll use golang.org/x/exp/slog for versions of Go before 1.21, and log/slog for versions after.

abhinav · 2023-08-16T14:12:02Z

exp/go.mod

-go 1.19
+go 1.21


Increasing the minimum required Go version isn't necessary if we're using //go:build directives to support exp/'s slog package for older versions.

abhinav · 2023-08-16T14:13:39Z

exp/zapslog/slog_old.go

@@ -0,0 +1,190 @@
+//go:build go1 && !go1.21


Typically, for Go-version-specific files, we've named them foo_pre_go$version and foo_go$version.
For example, we have https://github.com/uber-go/zap/blob/fcfd368624a286486e62d46deba7e42c20164616/array_go118.go.
(There's no pre-Go version for that.)

So this should probably be called slog_pre_go121.go.

abhinav · 2023-08-16T14:14:08Z

exp/zapslog/slog.go

@@ -1,3 +1,5 @@
+//go:build go1.21


And this should be called slog_go121.go.

abhinav · 2023-08-16T19:22:08Z

PR updated:

Merged master in so tests run against 1.20 and 1.21
Made the renames I suggested above
Lowered the go directive to Go 1.20
Updated doc.go as suggested above

abhinav

A couple comments probably out of scope for this PR,
but should be addressed before we consider upgrading this to the main Zap package.

Also, we should only add the log/slog-based version to the main Zap package.
The exp-based version should be deleted at that point.

abhinav · 2023-08-16T19:43:57Z

exp/zapslog/slog_go121.go

@@ -0,0 +1,190 @@
+// Copyright (c) 2023 Uber Technologies, Inc.


We probably need a slogtest to check that the handler is right.

https://pkg.go.dev/testing/slogtest#TestHandler

abhinav · 2023-08-16T19:44:42Z

exp/zapslog/slog_go121.go

+
+// WithAttrs returns a new Handler whose attributes consist of
+// both the receiver's attributes and the arguments.
+func (h *Handler) WithAttrs(attrs []slog.Attr) slog.Handler {


This should probably be covered before we consider upgrading this package to the main Zap module.

Signed-off-by: Juan C. Müller <[email protected]>

The exp package still supports Go 1.20. The std log/slog package will be used only on Go 1.21.

We place the build tag after the copyright notice.

This change makes the tests on 1.19 and 1.20 actually read these files Signed-off-by: Juan C. Müller <[email protected]>

Signed-off-by: Juan C. Müller <[email protected]>

jcmuller · 2023-08-17T13:21:54Z

Thanks, @abhinav. Just added a couple of commits fixing test and lint running.

Constraints don't need the go1.19 minimum. Just "anything before 1.21" is good enough.

abhinav · 2023-08-19T00:11:40Z

@sywhang @mway please merge when you have a chance. I can't do anything about the coverage error. I expect we should fix that when we move this to the non-exp Zap module.

mway · 2023-08-19T00:13:30Z

@sywhang @mway please merge when you have a chance. I can't do anything about the coverage error. I expect we should fix that when we move this to the non-exp Zap module.

Ack, no worries.

JacobOaks assigned sywhang Aug 15, 2023

JacobOaks requested a review from sywhang August 15, 2023 18:09

abhinav reviewed Aug 16, 2023

View reviewed changes

shreyner and others added 8 commits August 17, 2023 09:11

Update to work with released log/slog

e73c209

Work with older versions of golang

dcecb02

Signed-off-by: Juan C. Müller <[email protected]>

Lower minimum-required Go version to 1.20

1dde67e

The exp package still supports Go 1.20. The std log/slog package will be used only on Go 1.21.

Rename files to pre_go120 or go120, update doc.go

7507ce2

Move build tag below copyright notice

14f73c5

We place the build tag after the copyright notice.

Only 1.19 and up are supported

26ad5a6

This change makes the tests on 1.19 and 1.20 actually read these files Signed-off-by: Juan C. Müller <[email protected]>

Revert version bump

20f72ca

Signed-off-by: Juan C. Müller <[email protected]>

Update honnef tools

c5bd515

Signed-off-by: Juan C. Müller <[email protected]>

abhinav added 2 commits August 18, 2023 16:46

Merge branch 'master' into work-with-older-versions

62ba034

Fix renames (1.21, not 1.20), simplify constraints

4d5fbfb

Constraints don't need the go1.19 minimum. Just "anything before 1.21" is good enough.

abhinav approved these changes Aug 19, 2023

View reviewed changes

abhinav mentioned this pull request Aug 19, 2023

Update to work with released log/slog #1318

Closed

mway merged commit b454e18 into uber-go:master Aug 19, 2023
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to work with released log/slog #1320

Update to work with released log/slog #1320

jcmuller commented Aug 14, 2023 •

edited

Loading

CLAassistant commented Aug 14, 2023 •

edited

Loading

codecov bot commented Aug 15, 2023 •

edited

Loading

abhinav left a comment

abhinav Aug 16, 2023

abhinav Aug 16, 2023

abhinav Aug 16, 2023

abhinav commented Aug 16, 2023

abhinav left a comment •

edited

Loading

abhinav Aug 16, 2023

abhinav Aug 16, 2023

jcmuller commented Aug 17, 2023 •

edited

Loading

abhinav commented Aug 19, 2023

mway commented Aug 19, 2023

		@@ -0,0 +1,190 @@
		// Copyright (c) 2023 Uber Technologies, Inc.

Update to work with released log/slog #1320

Update to work with released log/slog #1320

Conversation

jcmuller commented Aug 14, 2023 • edited Loading

CLAassistant commented Aug 14, 2023 • edited Loading

codecov bot commented Aug 15, 2023 • edited Loading

Codecov Report

abhinav left a comment

Choose a reason for hiding this comment

abhinav Aug 16, 2023

Choose a reason for hiding this comment

abhinav Aug 16, 2023

Choose a reason for hiding this comment

abhinav Aug 16, 2023

Choose a reason for hiding this comment

abhinav commented Aug 16, 2023

abhinav left a comment • edited Loading

Choose a reason for hiding this comment

abhinav Aug 16, 2023

Choose a reason for hiding this comment

abhinav Aug 16, 2023

Choose a reason for hiding this comment

jcmuller commented Aug 17, 2023 • edited Loading

abhinav commented Aug 19, 2023

mway commented Aug 19, 2023

jcmuller commented Aug 14, 2023 •

edited

Loading

CLAassistant commented Aug 14, 2023 •

edited

Loading

codecov bot commented Aug 15, 2023 •

edited

Loading

abhinav left a comment •

edited

Loading

jcmuller commented Aug 17, 2023 •

edited

Loading