Skip to content

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

Merged
matthiasbeyer merged 1 commit into
rust-cli:masterfrom
chenl:issue-99
Feb 2, 2023
Merged

matthiasbeyer merged 1 commit into
rust-cli:masterfrom
chenl:issue-99

Conversation

@chenl

@chenl chenl commented Feb 27, 2019

Copy link
Copy Markdown

fix #99

@mehcode

mehcode commented May 9, 2019

Copy link
Copy Markdown
Collaborator

🙇‍♂️ 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
Copy Markdown

@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
Copy Markdown
Member

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
Copy Markdown

@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

matthiasbeyer commented Aug 20, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown

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

@szarykott

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

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

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

@matthiasbeyer
matthiasbeyer merged commit c71ad9c into rust-cli:master Feb 2, 2023
epage added a commit that referenced this pull request Sep 8, 2026
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [crate-ci/typos](https://redirect.github.com/crate-ci/typos) |
repository | minor | `v1.48.0` → `v1.50.0` |

Note: The `pre-commit` manager in Renovate is not supported by the
`pre-commit` maintainers or community. Please do not report any problems
there, instead [create a Discussion in the Renovate
repository](https://redirect.github.com/renovatebot/renovate/discussions/new)
if you have any questions.

---

### Release Notes

<details>
<summary>crate-ci/typos (crate-ci/typos)</summary>

###
[`v1.50.0`](https://redirect.github.com/crate-ci/typos/releases/tag/v1.50.0)

[Compare
Source](https://redirect.github.com/crate-ci/typos/compare/v1.49.1...v1.50.0)

#### \[1.50.0] - 2026-08-28

##### Features

- Updated the dictionary with the [August
2026](https://redirect.github.com/crate-ci/typos/issues/1587) changes

###
[`v1.49.1`](https://redirect.github.com/crate-ci/typos/releases/tag/v1.49.1)

[Compare
Source](https://redirect.github.com/crate-ci/typos/compare/v1.49.0...v1.49.1)

#### \[1.49.1] - 2026-08-27

##### Fixes

- Don't correct the brand name `HashiCorp`

###
[`v1.49.0`](https://redirect.github.com/crate-ci/typos/releases/tag/v1.49.0)

[Compare
Source](https://redirect.github.com/crate-ci/typos/compare/v1.48.0...v1.49.0)

#### \[1.49.0] - 2026-08-03

##### Features

- Updated the dictionary with the [July
2026](https://redirect.github.com/crate-ci/typos/issues/1573) changes

</details>

---

### Configuration

📅 **Schedule**: (UTC)

- Branch creation
  - "before 5am on the first day of the month"
- Automerge
  - At any time (no schedule defined)

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/epage/_rust).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC40OS4wIiwidXBkYXRlZEluVmVyIjoiNDQuNDkuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->
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.

the FileSource trate is not exposed in the documentation

5 participants