Replace `GetLastError` with `Error(win32: GetLastError())`
compnerd opened this issue · 8 comments
Converting the cases of GetLastError
to Error(windows: GetLastError())
will give a much better error diagnostic reporting the error code and a human readable message.
Would it be worth having a helper init/method/property in the Error Struct that would get the last error as part of it's function.
maybe a static func, something along the lines of:
static func lastError() -> Error { return Error(win32: GetLastError()) }
I don't think that is very good for readability - what is last error? Is it errno
or some other state? The reason for the explicit constructor for Error
here is to identify that you mean the Win32 error that you capture via the call.
I guess that makes sense, just feels wrong to be writing the same thing again and again and should use some sort of convenience method/helper, but I can’t think of a decent way of over coming it
I suppose that one way to do that would be to actually wrap the APIs and convert them to throwing versions, but then you end up with a lot of do { } catch { }
blocks and the wrapping functions. There isn't a very good way to handle that that I can see. But you will still have the throw Error(win32: GetLastError())
or throw Error(hr: hr)
.
For the moment as these are mainly being logged, using the constructor makes sense.
As the package matures it might make sense to treat functions that are likely to have errors as throwing and at that point it probably makes more sense to have explicit Error types with richer information
Yes, I agree. I think that's exactly the thing - as the project evolves, the needs will grow and that's the right time to handle them :). They also make it easier to design the solution as you have the concrete problem.
I will make the changes and try and get a PR over. Do you have any contribution guidelines?
I am running off of a Mac, so build may have to be done on PR
That sounds great! I've not yet drafted anything in terms of guidelines. Basically, in terms of naming, try to follow the windows conventions (Hungarian notation and all) when interacting with the windows side, and please stick to 80-columns.
I think that running the build test on CI for this change should be fine as it's very mechanical.