-
Notifications
You must be signed in to change notification settings - Fork 221
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
Conversation
🙇♂️ 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 { } |
@matthiasbeyer Could we revisit this please? |
I am actively thinking about this and still have this issue in my inbox, so you're not forgotten! 😄 |
@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. |
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 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 😆 ) |
@matthiasbeyer I'm still interested in this happening, fwiw. |
This should be more less neutral to changes I want to introduce. I might have to drop 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 So in my mind:
Structs that implement Then code to parse files and in-memory strings could be mutualized in the form of struct implementing 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 Apologize for this long text. Thank you for getting to the bottom ;) |
3753520
to
ae77ce6
Compare
I just updated this PR because it had conflicts. Do we still want this? |
Signed-off-by: Matthias Beyer <[email protected]>
ae77ce6
to
f59ffc9
Compare
Rebased, assuming we want this, so I'll merge after CI is green. |
fix #99