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

Add Error.is_os_error #89

Closed
wants to merge 2 commits into from
Closed

Add Error.is_os_error #89

wants to merge 2 commits into from

Conversation

citreae535
Copy link
Contributor

@citreae535 citreae535 commented Feb 7, 2023

This function checks if an error from win32 API call matches a specific OS error code. This makes it easier for users to check specific API errors.


This change is Reviewable

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Nice addition. A few leftovers in this PR just like in all others.

Reviewed 1 of 7 files at r1.
Reviewable status: 1 of 7 files reviewed, 1 unresolved discussion (waiting on @citreae535)


src/lib.rs line 272 at r1 (raw file):

    /// [official docs]: https://learn.microsoft.com/en-us/windows/win32/api/winsvc/#functions
    pub fn is_os_error(&self, error_code: i32) -> bool {
        if let Self::Winapi(e) = self {

I think historically we prefer to avoid return. This could have been written as a match statement instead, i.e:

match self {
    Self::Winapi(e) => e.raw_os_error() == Some(error_code),
    _ => false
}

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Looking good. Please rebase your PR.

Reviewed 1 of 7 files at r2.
Reviewable status: 1 of 8 files reviewed, all discussions resolved

@faern
Copy link
Member

faern commented Feb 13, 2023

I'm not against this PR. But I also want to note that this crate is supposed to be a high level abstraction for Windows services, and a user of the library should preferably not need to deal with OS error codes or other windows-sys stuff if possible.

@citreae535
Copy link
Contributor Author

Hmm, I reconsidered it and have to agree that most users won't need this change. Better close it then.

@citreae535 citreae535 closed this Feb 15, 2023
@citreae535 citreae535 deleted the error_helper branch February 15, 2023 12:17
@pronebird
Copy link
Contributor

Currently we do not translate errors returned from syscalls into meaningful errors. Instead we return Winapi(io::Error) variant so the one has to parse io::Error to find the cause of error.

We could of course introduce new error types and provide error variants for common error codes but we would almost always have to have a Other(io::Error) variant since we do not control Windows API. I am not quite sure that beefing up a single Error type would be the way to go, on the other side having dedicated errors types would mean that we'd have to have duplicate error variants like those that we currently have in the Error type.

Anyhow, reinventing that would be a big task.

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.

3 participants