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

Return all Windows partitions #1347

Merged
merged 2 commits into from Oct 9, 2022
Merged

Return all Windows partitions #1347

merged 2 commits into from Oct 9, 2022

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Sep 7, 2022

Return all partitions on Windows and all errors rather than returning early.

@atoulme
Copy link
Contributor Author

atoulme commented Sep 28, 2022

@shirou any chance you'd be available to review?

@shirou
Copy link
Owner

shirou commented Sep 28, 2022

Thank you for contribution(and sorry to late response)! Is the purpose of this PR to get all partition information in case of errors?
If so, I have the following comments.

  1. How about moving errorCollector under internal/common, since this code seems generic?
  2. Is the purpose achieved by using go-multierror library ?

@atoulme
Copy link
Contributor Author

atoulme commented Sep 29, 2022

Thanks for your reply!

  1. Sure thing
  2. That works too. I didn't know if it was a good idea to pull in a new library and tried to go for the most minimal approach instead. Would you like to use https://github.com/uber-go/multierr or https://github.com/hashicorp/go-multierror?

@shirou
Copy link
Owner

shirou commented Sep 30, 2022

Thank you.

You are correct that gopsutil is minimalist oriented. Therefore, I am happy not to use these libraries. However, if you could align the method names, etc. with these libraries (ex. Append, not collect) , it would make it easier for other users who have used these libraries before.

@Lomanic
Copy link
Collaborator

Lomanic commented Sep 30, 2022

I remember there already is a multierr-like thing in gopsutil though, maybe reuse or adapt that? https://github.com/shirou/gopsutil/blob/16b3aac6adb5ef7c2c30e1b1cce90c35069b829a/host/types.go

@shirou
Copy link
Owner

shirou commented Sep 30, 2022

Oh, Great! So, could you move this struct and functions to under internal and reuse it? Thanks!

@shirou
Copy link
Owner

shirou commented Oct 1, 2022

I found errors.Join() is now added to errors package. I don't know when it will be released. But, I would change what I mentioned before, it would be great if you could implement this error handling with the intention of replacing it with this errors.Join instead of multierr.

@atoulme
Copy link
Contributor Author

atoulme commented Oct 3, 2022

Please take a look. I have refactored Warnings to be part of internal/common.

@atoulme
Copy link
Contributor Author

atoulme commented Oct 5, 2022

@shirou please let me know?

Copy link
Owner

@shirou shirou left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much!

@klauspost
Copy link

@atoulme @shirou

For changes like these that break compatibility may I recommend you add type Warnings = common.Warnings with a deprecation notice?

@atoulme
Copy link
Contributor Author

atoulme commented Nov 30, 2022

First PR here. I had the assumption that given that this was an internal package, it wasn’t imported directly downstream. Sorry for the breakage.

@shirou
Copy link
Owner

shirou commented Nov 30, 2022

I have opened #1379 to fix. (type alias is better, thank you for your suggestion.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants