-
Notifications
You must be signed in to change notification settings - Fork 212
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 AsyncSource with tests, docs and examples #207
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
Overall this looks really nice and I like that you added a few tests as well! I hope we can get this in!
I have a few things to remark, though! 😄
src/source.rs
Outdated
@@ -12,23 +14,65 @@ pub trait Source: Debug { | |||
|
|||
/// Collect all configuration properties available from this source and return | |||
/// a HashMap. | |||
fn collect(&self) -> Result<HashMap<String, Value>>; | |||
fn collect(&self) -> Result<HashMap<String, Value>, ConfigError>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason why you don't use the crate::error::Result<_>
anymore? If no, please revert this (and all similar) changes, so we don't have to specify ConfigError
all the time in this module!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that Result
is not exported from the crate, which I remember vaguely did not play nicely with rust-analyzer that automatically used crate's method signature when asked to "implement trait functions". And since it wasn't exported, it didn't compile, I had to think about why. It was a long time ago, so it might be fixed now.
And personally, I am not a big fan of exporting Result, because in the case of import use config::*
it overwrites std's Result
and it is annoying. Hence the decision to change it here, since people have to implement it.
I am open to discussion though, it might be good idea to test it once more as well.
Thank you for the review! I'll try to fix things and answer in a few days. |
It seems there is a problem - due to I see few options:
Feel free to input your suggestions and in the meantime, I'll try to figure it out. |
Well, if we need a newer rust version because we need reqwest, the sensible thing would be to upgrade our MSRV to match that of reqwest. From what I see, though, this crate is only required in the examples. I would therefore limit the requirement in the What do you think? |
Try to cherry-pick matthiasbeyer@6f84f86 onto your branch if you like. This should fix the issue. (CI run here) |
Green. I dumped versions of packages that are only used in examples and tests to older versions that do not use fancy language features. |
Yeah, that change was unnecessary, please pull (fast-forward) from https://github.com/matthiasbeyer/config-rs/tree/pr-207 (or, if you don't mind, I can directly push my changes to this PR). I also had a quick look at the commit history and I'm not happy yet, sorry. As soon as I'm happy with the overall changes and did a review, we should clean the history. There are a lot of unnecessary changes and commits in the history which should be removed. I can offer you help with that, even doing the rebase myself if you are okay with it / don't have the time / whatever! Over all, though, I want to thank you very much for your patience and work on this crate! Async sources really is a feature we want to have here and I'm only so nit-picky here so we are doing the first steps right so we don't have to rewrite the feature later on (and that bit about the commit history is just my OCD on git history kicking in 😆 )! Please don't be offended by my nit-picking! |
@matthiasbeyer I'd be glad if you fixed the history in a way you see appropriate! Same goes for pushing the code you mentioned from your branch. |
Signed-off-by: Matthias Beyer <[email protected]>
1bcb015
to
9922425
Compare
Signed-off-by: Matthias Beyer <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
This patch adds the AsyncSource trait, the interface for providing async source functionality for this crate. Signed-off-by: Matthias Beyer <[email protected]> Reviewed-by: Matthias Beyer <[email protected]>
This patch rewrites the Config building mechanism using special objects for tracking the config building state. Transitions between states are done on the fly as required. This is required so that the async sources can be stored inside the configuration building objects, while keeping out the expenses in the non-async case, so a user of the crate has only to pay for what they are using (no async means no overhead for that). Signed-off-by: Matthias Beyer <[email protected]> Reviewed-by: Matthias Beyer <[email protected]>
9f2c585
to
8844f13
Compare
Feel free to review what I did and check whether I messed up somewhere... now this seems to be ready for merging (I'll do another review next week... too much to do today and in the next two days). |
It looks good to me |
Found some more things. Somehow the old |
Thank you. I pushed some commits to fix the pipeline. |
Signed-off-by: Matthias Beyer <[email protected]> Reviewed-by: Matthias Beyer <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]> Reviewed-by: Matthias Beyer <[email protected]>
This is required because the examples pull in the "reqwest" crate, which depends on "socket2" and this crate fails to build if the "const fn" feature is not present (which wasn't on 1.44.0). Hence, we only run the tests in the 1.44.0 job, but do not compile the examples. Signed-off-by: Matthias Beyer <[email protected]>
Because one of our dependencies (namingly `socket2`) uses match in a const fn, which is stabilized in rust 1.46.0, we bump to this version as MSRV for this crate. Signed-off-by: Matthias Beyer <[email protected]> Tested-by: Matthias Beyer <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
Signed-off-by: Matthias Beyer <[email protected]>
With this simplification, we save a bit of code on one side, but also showcase that errors from custom AsyncSource implementations are possible because the ConfigError type provides a variant for it. Signed-off-by: Matthias Beyer <[email protected]> Tested-by: Matthias Beyer <[email protected]>
This PR has come a long way and I really, really appreciate the work you've put in here. I'm merging now! 🎉 |
Resolves #148
This pull request adds a new type of source -
AsyncSource
, that supportsasync fn
. Also, it modifies the builder in order to handle it properly. Builder implementation based on this article.There is no implementation of such a source inside the library since there are various runtimes to choose from as explained in the rustdocs in this pull request.
One minor change - the type of
Result
in regularSource
has been changed from type synonym defined inside the library to std'sResult
since ... type synonym was not exported from the crate.Json5 tests have been modified for Windows. There was some issue with line endings I was not able to resolve in error message tests. CI for this package runs on Linux, so test will run.