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

use TotalXXX instead of NumXXX #381

Merged
merged 1 commit into from
Sep 25, 2024
Merged

use TotalXXX instead of NumXXX #381

merged 1 commit into from
Sep 25, 2024

Conversation

jaypipes
Copy link
Owner

All other fields that contain a total amount of something begin with the word Total except for the pkg/cpu.Processor.NumCores and NumThreads fields. This commit adds new fields to pkg/cpu.Processor called TotalCores and TotalHardwareThreads to bring the names of these fields in-line with all other amount/total fields.

In addition, adds a TotalHardwareThreads field to the pkg/cpu.Info struct and deprecates the less-explicit TotalThreads field.

I'd like to remove the now-deprecated NumThreads, NumCores, and TotalThreads fields before we get to a v.1.0 series.

All other fields that contain a total amount of something begin with the
word `Total` except for the `pkg/cpu.Processor.NumCores` and
`NumThreads` fields. This commit adds new fields to `pkg/cpu.Processor`
called `TotalCores` and `TotalHardwareThreads` to bring the names of
these fields in-line with all other amount/total fields.

In addition, adds a `TotalHardwareThreads` field to the `pkg/cpu.Info`
struct and deprecates the less-explicit `TotalThreads` field.

I'd like to remove the now-deprecated `NumThreads`, `NumCores`, and
`TotalThreads` fields before we get to a `v.1.0` series.

Signed-off-by: Jay Pipes <[email protected]>
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.

  • we do change the docs, but that's fine, we're recommending the new fields
  • we keep setting correctly the old fields for backward compat

we do change the JSON representation though, do we need to test/guarantee the JSON roundtrip?
@jaypipes feel free to merge anytime if the JSON representation is not a concern for backward compat

@jaypipes
Copy link
Owner Author

LGTM.

* we do change the docs, but that's fine, we're recommending the new fields

* we keep setting correctly the old fields for backward compat

we do change the JSON representation though, do we need to test/guarantee the JSON roundtrip? @jaypipes feel free to merge anytime if the JSON representation is not a concern for backward compat

@ffromani yeah, see the issue I created yesterday about the JSON/YAML representation and firming that up with a published JSONSchema. :)

@jaypipes
Copy link
Owner Author

@ffromani also, note that I only added fields to the JSON output in this PR. It's still backwards-compat representation of the JSON/YAML but I do want to deprecate things before v1.0

@jaypipes jaypipes merged commit 2950821 into main Sep 25, 2024
14 checks passed
@jaypipes jaypipes deleted the num-total branch September 25, 2024 11:41
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