rust-lang / jobserver-rs

Apache License 2.0
69 stars 39 forks source link

Error type for `from_env_ext` function #54

Closed belovdv closed 1 year ago

belovdv commented 1 year ago

Continue implementing #51. The previous PR wasn't finished and cleaned up.

If we want to discern different errors and treat them differently, more specific return type would be more convenient.

If this's going to be merged, probably there'll be some changes from current version. If it's possible, i'd like to clean up results before merging.

petrochenkov commented 1 year ago

ping @alexcrichton for merging ping @weihanglo in case you have any additional comments

(So far this PR has consensus between me, @NobodyXu and @belovdv.)

petrochenkov commented 1 year ago

The FromEnv structure feels pretty unidiomatic as it's a struct-containing-a-Result which breaks ? usage and requires filling out fields that may not always make the most sense.

? is not exactly broken because Client::from_env_ext().client? would still work.

But it's not too important anyway, this function is called ~once in a program, and if someone is calling Client::from_env_ext() instead of Client::from_env() he's likely going to inspect and/or log all those errors and env vars.

FromEnv::var is not inside the Result because it may exist regardless of the client being successfully constructed or not.

alexcrichton commented 1 year ago

I'll reiterate the second part of my comment above. I realize that this is a minor crate which is not really all that heavily used. That being said I do not want to personally maintain the idioms introduced here, nor do I necessarily have the time or energy to walk through the review process towards something I would be willing to maintain. If updating this PR requires lots of feedback from me, or if this PR would rather not be updated, then I think it's best to look for a different maintainer of this crate to transfer ownership to.

petrochenkov commented 1 year ago

This PR would rather not be updated. I'll make a proposal to move this crate to rust-lang organization.