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

Error type for from_env_ext function #54

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

belovdv
Copy link
Contributor

@belovdv belovdv commented Apr 11, 2023

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.

@belovdv belovdv marked this pull request as draft April 11, 2023 14:39
@belovdv belovdv changed the title error type for from_env_ext function Error type for from_env_ext function Apr 11, 2023
@belovdv belovdv marked this pull request as ready for review April 11, 2023 15:19
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

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

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

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Sorry I haven't been following the discussion here and it seems like there's a good deal, but this to me feels like it's dialing the wrong knob per se up to 11. 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. I can sort of see how it's conveying more information but I would not personally settle on this particular design.

On one hand this is fine. This isn't exactly a heavily used crate by everyone in every project so it's not like something that's a bit unidiomatic will make that much of a difference. On the other hand, though, this is not code that I would want to maintain. I would prefer to improve the idioms here and figure out a different way which I'm more comfortable with which achieves the same goal. Unfortunately I don't have a ton of time or energy to go through this process. At this point it may be best for me to hand off this crate to someone else, perhaps rust-lang, since it's depended on by rustc and cargo. I at the same time don't want to block progress on feature work here.

@petrochenkov
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

Looks fine with typo fixed.

src/lib.rs Outdated Show resolved Hide resolved
@belovdv belovdv force-pushed the err-extend-type branch from 028096e to 3c0739e Compare July 5, 2023 17:04
@petrochenkov petrochenkov merged commit 60e5a33 into rust-lang:main Jul 5, 2023
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.

5 participants