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 and const cleanup for cpu, block, memory, and net packages #352

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

jaypipes
Copy link
Owner

doc and const cleanup for net package

Ensures proper docstrings for all exported things in the `net`
package and deprecates the (non-Go-idiomatic) ALL_CAPS constants in
favour of CamelCase constants.

doc and const cleanup for block package

Ensures proper docstrings for all exported things in the `block`
package and deprecates the (non-Go-idiomatic) ALL_CAPS constants in
favour of CamelCase constants.

Also officially deprecates the `pkg/block.Info.TotalPhysicalBytes` field
in favour of a `pkg/block.Info.TotalSizeBytes` field (had a TODO in
there a while to do this...).

ensure ProcessorCore.LogicalProcessors sorted

Ensures that we sort the `ProcessorCore.LogicalProcessors` slice of ints
for each processor core in Linux.

Also update the documentation for the CPU package to mark the
`Processor.Cores` and `Processor.Capabilities` fields as Linux-only.

doc and const cleanup for memory package

Ensures proper docstrings for all exported things in the `memory`
package and deprecates the (non-Go-idiomatic) ALL_CAPS constants in
favour of CamelCase constants.

Ensures proper docstrings for all exported things in the `memory`
package and deprecates the (non-Go-idiomatic) ALL_CAPS constants in
favour of CamelCase constants.

Signed-off-by: Jay Pipes <jaypipes@gmail.com>
Ensures that we sort the `ProcessorCore.LogicalProcessors` slice of ints
for each processor core in Linux.

Also update the documentation for the CPU package to mark the
`Processor.Cores` and `Processor.Capabilities` fields as Linux-only.

Signed-off-by: Jay Pipes <jaypipes@gmail.com>
Ensures proper docstrings for all exported things in the `block`
package and deprecates the (non-Go-idiomatic) ALL_CAPS constants in
favour of CamelCase constants.

Also officially deprecates the `pkg/block.Info.TotalPhysicalBytes` field
in favour of a `pkg/block.Info.TotalSizeBytes` field (had a TODO in
there a while to do this...).

Signed-off-by: Jay Pipes <jaypipes@gmail.com>
Ensures proper docstrings for all exported things in the `net`
package and deprecates the (non-Go-idiomatic) ALL_CAPS constants in
favour of CamelCase constants.

Signed-off-by: Jay Pipes <jaypipes@gmail.com>
@jaypipes jaypipes requested a review from ffromani July 22, 2023 13:27
@jaypipes
Copy link
Owner Author

@ffromani mostly docstring updates in here. sorry for the large PR. Feel free to review each commit separately if that's easier for you.

@jaypipes
Copy link
Owner Author

@ffromani heya :) any chance to do a quick review on this one today?

@ffromani
Copy link
Collaborator

@ffromani heya :) any chance to do a quick review on this one today?

yes, sorry. I'll squeeze in my day today

@@ -112,6 +113,9 @@ func processorsGet(ctx *context.Context) []*Processor {
}
res := []*Processor{}
for _, p := range procs {
for _, c := range p.Cores {
sort.Ints(c.LogicalProcessors)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't point my finger at anything but this looks suspicious. Let's monitor CI after we merge this PR. Not stopping because I can't back my gut feeling with any hard data.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's just sorting the logical processor indexes to avoid situations like here where I thought there was a bug because the logical processors listed in a non-sorted way :)

Copy link
Collaborator

@ffromani ffromani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

thanks for your work! impressive

"nvme": StorageControllerNVMe,
"virtio": StorageControllerVirtIO,
"mmc": StorageControllerMMC,
"loop": StorageControllerLoop,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we use reverse table like this I'm not sure the backward compatibility is ensured, but I think this should not be a bother. We can break stuff moving to 1.0.0 (and then don't for a long long while)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change any string constants. Only the exported names of the various constants (but kept the old ALL_CAPS ones so that existing installations that may have referenced those constant names won't break)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I misread that part. So we're golden!

@ffromani ffromani merged commit 73f5a04 into main Jul 26, 2023
10 checks passed
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.

2 participants