.*: fix lint issues of not having comments for exported funcs and vars along with any remaining issues and enable remaining disabled rules #7575

purnesh42H · 2024-08-30T13:36:30Z

Addresses: #7444

RELEASE NOTES: None

codecov · 2024-08-30T13:39:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.73%. Comparing base (31ffeee) to head (5aefed2).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7575   +/-   ##
=======================================
  Coverage   81.73%   81.73%           
=======================================
  Files         361      361           
  Lines       27817    27816    -1     
=======================================
+ Hits        22735    22736    +1     
- Misses       3871     3872    +1     
+ Partials     1211     1208    -3

Files with missing lines	Coverage Δ
balancer/endpointsharding/endpointsharding.go	`66.92% <ø> (ø)`
balancer/pickfirst/pickfirst.go	`83.73% <ø> (ø)`
credentials/tls/certprovider/pemfile/builder.go	`100.00% <ø> (ø)`
internal/balancer/gracefulswitch/config.go	`82.60% <ø> (ø)`
internal/channelz/channel.go	`79.80% <ø> (ø)`
internal/channelz/server.go	`90.69% <ø> (ø)`
internal/channelz/socket.go	`46.66% <ø> (ø)`
internal/channelz/subchannel.go	`96.00% <ø> (ø)`
internal/channelz/trace.go	`92.15% <ø> (ø)`
internal/idle/idle.go	`89.15% <ø> (ø)`
... and 5 more

... and 8 files with indirect coverage changes

balancer/endpointsharding/endpointsharding.go

credentials/tls/certprovider/pemfile/builder.go

internal/channelz/server.go

internal/channelz/socket.go

dfawley · 2024-08-30T16:14:31Z

internal/channelz/trace.go

+	// A slice of traceEvent pointers representing the events recorded for
+	// this channel.
+	Events []*traceEvent


Hmm, should this be unexported? Its type is unexported, which is an anti-pattern. It's probably unused outside this package would be my guess/hope.

its being used in channelz/internal/protoconv

That's not good. We should export traceEvent, then. If that's too hard to do in this PR, it's fine to leave it for now.

this traceEvent is an internal representation of a single trace event. There is already an exported TraceEvent which is used in clientconn. We will have to give it a different name like TraceEventInternal

That's not a very descriptive choice, but yes, some other name.

Is there any linter that covers unexported types being exposed outside a package, like this case? If so I would like to turn it on.

there is no built-in rule but revive provides framework to write custom rules or we can ask revive contributors for this

internal/testutils/stats/test_metrics_recorder.go

internal/transport/transport.go

dfawley · 2024-08-30T16:17:08Z

mem/buffers.go

@@ -119,20 +122,25 @@ func Copy(data []byte, pool BufferPool) Buffer {
 	return NewBuffer(buf, pool)
 }

+// ReadOnlyData returns the underlying byte slice of the Buffer.


Similar as above. This type is unexported; these methods should not need comments.

alialobidm

dfawley · 2024-08-30T21:49:49Z

mem/buffers.go

+// emptyBuffer is a Buffer implementation that represents an empty buffer. All
+// methods are no-op implementations.


Please remove.

dfawley · 2024-08-30T21:49:58Z

mem/buffers.go

 type emptyBuffer struct{}

 func (e emptyBuffer) ReadOnlyData() []byte {
 	return nil
 }
-


Please undo this diff.

mem/buffers.go

scripts/vet.sh

arvindbr8

Overall LGTM, modulo a few comments and nits.

arvindbr8 · 2024-09-12T17:17:48Z

examples/features/advancedtls/client/main.go

+// Binary client is an example client demonstrating use of advancedtls,
+// to set up a secure gRPC client connection with various TLS authentication
+// methods.


nit:

Suggested change

// Binary client is an example client demonstrating use of advancedtls,

// to set up a secure gRPC client connection with various TLS authentication

// methods.

// Binary client is an example client demonstrating use of advancedtls, to set

// up a secure gRPC client connection with various TLS authentication methods.

arvindbr8 · 2024-09-12T17:19:21Z

internal/channelz/channel.go

 func (c *Channel) SubChans() map[int64]string {
 	db.mu.RLock()
 	defer db.mu.RUnlock()
 	return copyMap(c.subChans)
 }

+// NestedChans returns a copy of the map of nested channels associated with the Channel.


pls wrap at 80 col

Suggested change

// NestedChans returns a copy of the map of nested channels associated with the Channel.

// NestedChans returns a copy of the map of nested channels associated with the

// Channel.

arvindbr8 · 2024-09-12T17:19:41Z

internal/channelz/channel.go

@@ -61,24 +65,29 @@ func (c *Channel) id() int64 {
 	return c.ID
 }

+// SubChans returns a copy of the map of sub-channels associated with the Channel.


pls wrap at 80 col:

Suggested change

// SubChans returns a copy of the map of sub-channels associated with the Channel.

// SubChans returns a copy of the map of sub-channels associated with the

// Channel.

arvindbr8 · 2024-09-12T17:20:21Z

internal/channelz/channel.go

+// String returns a string representation of the ChannelMetrics, including its state,
+// target, and call metrics.


Suggested change

// String returns a string representation of the ChannelMetrics, including its state,

// target, and call metrics.

// String returns a string representation of the ChannelMetrics, including its

// state, target, and call metrics.

arvindbr8 · 2024-09-12T17:20:40Z

internal/channelz/channel.go

+// NewChannelMetricForTesting creates a new instance of ChannelMetrics with specified
+// initial values for testing purposes.


Suggested change

// NewChannelMetricForTesting creates a new instance of ChannelMetrics with specified

// initial values for testing purposes.

// NewChannelMetricForTesting creates a new instance of ChannelMetrics with

// specified initial values for testing purposes.

arvindbr8 · 2024-09-12T17:33:27Z

internal/testutils/stats/test_metrics_recorder.go

+// WaitForInt64Histo waits for an int histo metric to be recorded and
+// verifies that the recorded metrics data matches the expected
+// metricsDataWant.


Suggested change

// WaitForInt64Histo waits for an int histo metric to be recorded and

// verifies that the recorded metrics data matches the expected

// metricsDataWant.

// WaitForInt64Histo waits for an int histo metric to be recorded and verifies

// that the recorded metrics data matches the expected metricsDataWant.

arvindbr8 · 2024-09-12T17:35:58Z

mem/buffers.go

@@ -194,19 +197,19 @@ func (b *buffer) read(buf []byte) (int, Buffer) {
 	return n, b
 }

-// String returns a string representation of the buffer. May be used for
-// debugging purposes.
 func (b *buffer) String() string {


mmm, interesting that this one is removed.

yeah I was removing the docstrings for non exported structs

arvindbr8 · 2024-09-12T17:36:33Z

rpc_util.go

 			}
+			return status.Newf(codes.Internal, "grpc: Decompressor is not installed for grpc-encoding %q", recvCompress)


This seems unrelated?

it fixes one lingering superflous-else

arvindbr8 · 2024-09-12T17:38:02Z

scripts/revive.toml

-[rule.indent-error-flow]
-    Disabled = true # TODO: Enable after existing issues are fixed
+
+


nix new line pls

arvindbr8 · 2024-09-12T17:38:08Z

scripts/revive.toml

-[rule.exported]
-    Disabled = true # TODO: Enable after existing issues are fixed
-[rule.redefines-builtin-id]
-    Disabled = true # TODO: Enable after existing issues are fixed
-[rule.package-comments]
-    Disabled = true # TODO: Enable after existing issues are fixed
-[rule.indent-error-flow]
-    Disabled = true # TODO: Enable after existing issues are fixed


Could we keep this PR focused on that change and address the other removals in a separate PR? This will make it easier to track the history of these changes in the future.

I think its fine to just enable the remaining ones with this PR since at this time we are sure there are no more existing issues to be fixed

Modifying the PR title and merge commit to specify the PR also enable remaining disabled rules

…ed funcs and vars

…ed types

purnesh42H mentioned this pull request Aug 30, 2024

Add godoc linter #7444

Closed

13 tasks

purnesh42H force-pushed the fix-revive-exported-lints branch from a0d509d to bf84b2f Compare August 30, 2024 13:42

purnesh42H assigned arvindbr8 and dfawley Aug 30, 2024

purnesh42H added the Type: Documentation Documentation or examples label Aug 30, 2024

purnesh42H added this to the 1.67 Release milestone Aug 30, 2024

dfawley reviewed Aug 30, 2024

View reviewed changes

alialobidm reviewed Aug 30, 2024

View reviewed changes

arvindbr8 assigned purnesh42H and unassigned arvindbr8 and dfawley Aug 30, 2024

purnesh42H force-pushed the fix-revive-exported-lints branch from ea50b5f to 18b8820 Compare August 30, 2024 18:26

purnesh42H requested a review from dfawley August 30, 2024 18:28

purnesh42H assigned dfawley and unassigned purnesh42H Aug 30, 2024

purnesh42H force-pushed the fix-revive-exported-lints branch from 18b8820 to 551ffd4 Compare August 31, 2024 09:33

dfawley requested changes Sep 3, 2024

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Sep 3, 2024

purnesh42H requested a review from dfawley September 3, 2024 19:22

purnesh42H assigned dfawley and unassigned purnesh42H Sep 3, 2024

purnesh42H force-pushed the fix-revive-exported-lints branch from 7ee93ab to c4692da Compare September 3, 2024 19:26

arvindbr8 reviewed Sep 3, 2024

View reviewed changes

scripts/vet.sh Outdated Show resolved Hide resolved

purnesh42H requested review from arvindbr8 and alialobidm September 3, 2024 20:23

purnesh42H force-pushed the fix-revive-exported-lints branch from 767a0f0 to 8dad889 Compare September 5, 2024 04:25

arvindbr8 mentioned this pull request Sep 6, 2024

vet: enforce revive linter #7589

Merged

purnesh42H force-pushed the fix-revive-exported-lints branch from 8dad889 to 9f5e76e Compare September 6, 2024 15:48

dfawley removed their assignment Sep 10, 2024

purnesh42H mentioned this pull request Sep 12, 2024

.*: fix revive lints redefines-builtin-id #7552

Merged

purnesh42H force-pushed the fix-revive-exported-lints branch 2 times, most recently from db453a2 to a1861c3 Compare September 12, 2024 15:38

purnesh42H requested a review from dfawley September 12, 2024 15:41

purnesh42H assigned dfawley and unassigned purnesh42H Sep 12, 2024

dfawley approved these changes Sep 12, 2024

View reviewed changes

dfawley assigned purnesh42H and unassigned dfawley Sep 12, 2024

arvindbr8 approved these changes Sep 12, 2024

View reviewed changes

arvindbr8 added the Status: Requires Reporter Clarification label Sep 12, 2024

purnesh42H added 12 commits September 14, 2024 00:44

.*: fix revive exported lint issues of not having comments for export…

95776c7

…ed funcs and vars

addressing comments for not writing docstring for funcs with unexport…

d60462a

…ed types

Parse config docstring

25f9a7a

fix grammer and undo unncessary diffs

6b6af5b

fix comments upto 80 col

5bf431b

add package-comments to advancedtls example

49ab5e5

fix superflous-else in rpc-util

7f666f9

enable package-comments and indent-error0flow

08ef901

enable export

5a961d9

update comment for endpointsharding lb policy

b7dc304

enable redredefines-builtin-id lint rule

c65d011

addressing nits ing dosctrings

5aefed2

purnesh42H force-pushed the fix-revive-exported-lints branch from a1861c3 to 5aefed2 Compare September 13, 2024 19:27

purnesh42H changed the title ~~.*: fix revive lint issues of not having comments for exported funcs and vars~~ .*: fix and enable revive lint issues of not having comments for exported funcs and vars along with any remaining issues Sep 14, 2024

purnesh42H changed the title ~~.*: fix and enable revive lint issues of not having comments for exported funcs and vars along with any remaining issues~~ .*: fix lint issues of not having comments for exported funcs and vars along with any remaining issues and enable all rules Sep 14, 2024

purnesh42H merged commit 04e78b0 into grpc:master Sep 16, 2024
14 checks passed

purnesh42H mentioned this pull request Sep 23, 2024

cleanup: fix docstring style #7643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.*: fix lint issues of not having comments for exported funcs and vars along with any remaining issues and enable remaining disabled rules #7575

.*: fix lint issues of not having comments for exported funcs and vars along with any remaining issues and enable remaining disabled rules #7575

purnesh42H commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

dfawley Aug 30, 2024

purnesh42H Aug 30, 2024

dfawley Sep 10, 2024

purnesh42H Sep 12, 2024 •

edited

Loading

dfawley Sep 12, 2024

purnesh42H Sep 13, 2024

dfawley Aug 30, 2024

alialobidm left a comment •

edited by purnesh42H

Loading

dfawley Aug 30, 2024

purnesh42H Sep 3, 2024

dfawley Aug 30, 2024

purnesh42H Sep 3, 2024

arvindbr8 left a comment

arvindbr8 Sep 12, 2024

arvindbr8 Sep 12, 2024

arvindbr8 Sep 12, 2024

arvindbr8 Sep 12, 2024

arvindbr8 Sep 12, 2024

arvindbr8 Sep 12, 2024

arvindbr8 Sep 12, 2024

purnesh42H Sep 13, 2024

arvindbr8 Sep 12, 2024

purnesh42H Sep 13, 2024

arvindbr8 Sep 12, 2024

purnesh42H Sep 13, 2024

arvindbr8 Sep 12, 2024

purnesh42H Sep 13, 2024

purnesh42H Sep 16, 2024

		// emptyBuffer is a Buffer implementation that represents an empty buffer. All
		// methods are no-op implementations.

	// NestedChans returns a copy of the map of nested channels associated with the Channel.
	// NestedChans returns a copy of the map of nested channels associated with the
	// Channel.

	// SubChans returns a copy of the map of sub-channels associated with the Channel.
	// SubChans returns a copy of the map of sub-channels associated with the
	// Channel.

		// String returns a string representation of the ChannelMetrics, including its state,
		// target, and call metrics.

		// NewChannelMetricForTesting creates a new instance of ChannelMetrics with specified
		// initial values for testing purposes.

		}
		return status.Newf(codes.Internal, "grpc: Decompressor is not installed for grpc-encoding %q", recvCompress)

		[rule.indent-error-flow]
		Disabled = true # TODO: Enable after existing issues are fixed

.*: fix lint issues of not having comments for exported funcs and vars along with any remaining issues and enable remaining disabled rules #7575

.*: fix lint issues of not having comments for exported funcs and vars along with any remaining issues and enable remaining disabled rules #7575

Conversation

purnesh42H commented Aug 30, 2024 • edited Loading

codecov bot commented Aug 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alialobidm left a comment • edited by purnesh42H Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindbr8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

purnesh42H Sep 12, 2024 •

edited

Loading

alialobidm left a comment •

edited by purnesh42H

Loading