Misleading documentation on status.FromError

[yvesf]

The current documentation is:

// FromError returns a Status representing err if it was produced from this
// package or has a method `GRPCStatus() *Status`. Otherwise, ok is false and a
// Status is returned with codes.Unknown and the original error message.

The first statement is not true because the implementation does return nil, true if the err is nil.

The documentation could be improved to avoid that this function is used in wrong ways when not looking at the actual implementation.

What did you expect to see?

// FromError returns a Status representing err if it was produced from this
// package or has a method `GRPCStatus() *Status`. 
// If err is nil then Status nil will be returned and ok is true. Otherwise, ok is false and a
// Status is returned with codes.Unknown and the original error message.

[dfawley]

Thanks for the suggestion. I would probably tweak that a bit and write it as:

// FromError ...
// If err is nil, a Status is returned with codes.OK and no message.
// Otherwise, ok is false and ...

That this happens by returning nil is irrelevant and should not be relied upon.

[yvesf]

@dfawley I think your changes make a lot of sense because they mention what it is really about: the returned nil will claim "codes.OK" regardless if that was really the code and that's not necessarily what a programmer expect when calling with err=nil.

I would write it then like that (complete docstring):

// FromError returns a Status representing err if it was produced from this
// package or has a method `GRPCStatus() *Status`. 
// If err is nil, ok is true and a nil-Status is returned with codes.OK and empty message.
// Otherwise, ok is false and a Status is returned with 
// codes.Unknown and the original error message.

[dfawley]

My point is: the pointer returned by FromError shouldn't be inspected at all. There should be no need to do so. Documenting the inner-workings of this is unnecessary. Users should call its methods, Code and Message, instead.

[gauravgahlot]

@dfawley Can I take this up?

[dfawley]

Thanks, @gauravgahlot