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

Fix #99: expose file::source::FileSource #100

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

chenl
Copy link

@chenl chenl commented Feb 27, 2019

fix #99

@mehcode
Copy link
Owner

mehcode commented May 9, 2019

🙇‍♂️ Thank you for your contribution. I apologize for the late response. Moving forward I want to re-think the design of config as a whole. See #111 for more details.


That was intentional actually. It's a common idiom in Rust modules for "sealed" traits. The idiom has evolved slightly for better UX.

mod private { 
    pub trait Sealed { }
    impl<T> Sealed for T where T: super::FileSource { }
}

// Appears in docs but remains illegal to implement in downstream crates
pub trait FileSource: private::Sealed { }

@mehcode mehcode closed this May 9, 2019
@bitemyapp
Copy link

@matthiasbeyer Could we revisit this please? FileSource being sealed forced me to copy code between two functions, one that sourced a Config from a file and one that source it from a configuration string. I'd like to factor out the commonalities again if I can.

@matthiasbeyer
Copy link
Collaborator

I am actively thinking about this and still have this issue in my inbox, so you're not forgotten! 😄
I did not yet find the time to do some research in the codebase to make up my mind, I'm sorry about that!

@bitemyapp
Copy link

@matthiasbeyer No problem, I appreciate the ping/update. In the past to be cautious I've exported things like that under an "internal" module to mark what isn't formally supported or incorporated into the semver considerations.

@matthiasbeyer matthiasbeyer reopened this Aug 20, 2021
@matthiasbeyer
Copy link
Collaborator

matthiasbeyer commented Aug 20, 2021

So, I made up my mind. It seems that this might make downstream users happier. Thing is: If we encounter problems with this type being exposed, we're still in version 0.x.y here and can break the API if we need to, according to semver.

That said, I really don't like to break semver if not necessary, even in minor releases.

I'd like to get feedback from @szarykott still, how this would interact with all the changes they have done and are planning to do (also in the light of their open PRs). Other users are welcome to comment too, of course!


This might need a rebase, I can do that myself if OP doesn't respond or care anymore (which is of course completely ok 😆 )

@bitemyapp
Copy link

@matthiasbeyer I'm still interested in this happening, fwiw.

@szarykott
Copy link
Contributor

@matthiasbeyer

This should be more less neutral to changes I want to introduce. I might have to drop pub(crate) here and there, though.

I am wondering however what path should be taken for the library.

I opened #219, because I wanted to make the crate more generic. I try to look at the configuration in a more generic way than just files, idea I had in mind was to carve out "in memory string configuration" (dubbed FileSourceString in the code) from FileSource entirely, making it rely only on Format trait (because file extensions do not make any sense in the context of in memory-strings), which would allow to much simplify FileSource implementation (and maybe even remove the trait completely).

So in my mind:

  • struct FileSource - depends on Format trait and FileExtensions trait to customize behavior
  • struct InMemorySource (name should probably change) - depends only of Format

Structs that implement Format know how to turn &str into configuration, those that implement FileExtensions can plug into automatic file discovery of current FileSource. For example Json struct would know how to parse jsons and would know its extensions (.json 😅).

Then code to parse files and in-memory strings could be mutualized in the form of struct implementing Format and FileExtension. I see it as a bit cleaner than exposing FileFormat trait.

ref. from #219

pub trait Format {
    fn parse(
        &self,
        uri: Option<&String>,
        text: &str,
    ) -> Result<HashMap<String, Value>, Box<dyn Error + Send + Sync>>;
}>>

pub trait FileExtensions {
    /// Returns a vector of file extensions, for instance `[yml, yaml]`.
    fn extensions(&self) -> &Vec<&'static str>;
}

But I am open to discussion, to be honest I'd like to get my idea reviewed. What do you think @bitemyapp, would it solve your problem, can you share code samples of code you need to duplicate?

In simple words, I'd like to make this library as extensible (pluggable) as possible. My inspiration is excellent .NET Core's IConfiguration interface that is extremely extensible, provides the pipeline in which you can plug your code in.

Apologize for this long text. Thank you for getting to the bottom ;)

@matthiasbeyer
Copy link
Collaborator

I just updated this PR because it had conflicts. Do we still want this?

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@matthiasbeyer
Copy link
Collaborator

Rebased, assuming we want this, so I'll merge after CI is green.

@matthiasbeyer matthiasbeyer merged commit c71ad9c into mehcode:master Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted PR for hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

the FileSource trate is not exposed in the documentation
5 participants