-
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
Introduce Format
trait
#219
Conversation
It seems I will have to check Clippy on nightly. |
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.
First of all: Sorry for not coming back to this, I had a lot on my mind lately (and still have, so another roundtrip might take some time again, too... sorry about that)!
So for the PR: Well, this is quite the change, isn't it? I am not yet sure how this all will play out in production code later... Maybe I can find some time to actually try to move my production codebase to use this changeset to see how it works out - but that might take a fair bit of time.
I hope you don't mind this living for a bit longer... 😔
examples/custom_format/main.rs
Outdated
// It is only required for File source, custom sources can use Format without caring for extensions | ||
static MY_FORMAT_EXT: Vec<&'static str> = vec![]; | ||
impl FileExtensions for MyFormat { | ||
fn extensions(&self) -> &Vec<&'static str> { |
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.
So if we have a &'static str
here, wouldn't it be possible to remove the (heap allocated, if I'm not mistaken) Vec
here as well?
Maybe with something like a fn extensions(&self) -> impl Iterator<Item = &'static str>
kind of signature?
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.
Good point with the impl
, more flexible! Static lifetime should be enough, file extensions are quite ... static.
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.
To avoid heap allocation, I need to go with &'static [&'static str]
.
Why?
impl Iterator<Item = &'static str>
is forbidden in traits- to define associated type Iterator<Item = &'static str> I'd need to return a reference to a slice and I think that to pass lifetime to it I'd need GAT.
src/file/extension.rs
Outdated
/// Since [`Format`](crate::Format) only describes certain internal encoding, for instance JSON or Yaml | ||
/// it is not necessarily bound to file extension name. | ||
/// | ||
///In networking context JSONs are used without file extensions. |
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.
nit: missing leading space
src/file/format/mod.rs
Outdated
@@ -93,18 +96,18 @@ lazy_static! { | |||
impl FileFormat { | |||
// TODO: pub(crate) | |||
#[doc(hidden)] | |||
pub fn extensions(self) -> &'static Vec<&'static str> { | |||
pub fn extensions(&self) -> &'static Vec<&'static str> { |
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.
There's a TODO: pub(crate)
here, that could be resolved right away, right? 😄
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.
Sure, I'll see what I can do
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.
Required some changes in the example, but it is all pub (crate) now.
src/file/mod.rs
Outdated
pub fn from_str(s: &str, format: FileFormat) -> Self { | ||
impl<F> File<source::string::FileSourceString, F> | ||
where | ||
F: Format + FileExtensions + 'static, |
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 wonder how the 'static
bound behaves in production code then... 🤔
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.
It only disallows F
implementations to have borrowed fields, it can be easily worked around with a simple clone
or Rc
. Unless somebody adds sources to config in a hot loop in a critical path, this should be fine.
@matthiasbeyer I'll try to answer your concerns about production here. Previously As I see it, it only allows custom formats to be defined and integrated into library by users without breaking backwards compatibility. And I do not mind waiting, open source is goverened by its own rules. |
fad912e
to
c1002cb
Compare
01bea1c
to
9e5e2f8
Compare
Hey, @matthiasbeyer I pushed a lot of changes to make things green. What do you think about this PR now? |
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.
So from my view this looks good... I did not see any issues ... Do you think we can merge? Would you want more review (by others maybe)?
/// It also allows specifying optional URI of the source associated with format instance that can facilitate debugging. | ||
fn parse( | ||
&self, | ||
uri: Option<&String>, |
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.
Is there any particular reason this is a &String
and not a &str
?
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.
Ah, after some investigation I can see that the Value::new()
fn gets a Option<&String>
and clone()
s it to keep the origin of the value.
We might change that later on (because clone
ing down there is probably a performance issue...
For now that's of course okay.
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.
AFAIK this was something from API that was already existing, I just exported it into a trait. But indeed, now that you mention it, it looks strange.
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 investigated a bit yesterday, will open an issue about that.
@matthiasbeyer I'd like to give it one more look myself, but a review from somebody else would not be a bad idea. Are there people willing to do it? |
Awesome! If someone else takes a look, I'd love that (don't know who could, though 😄). Looking forward to merging this! |
src/file/extension.rs
Outdated
/// One can also imagine some encodings that do not necessarily have file extension associated, for instance | ||
/// MessagePack or bincode. | ||
/// Hence the decision to have extensions separated from [`Format`](crate::Format). | ||
pub trait FileExtensions { |
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.
@matthiasbeyer Tell me what you think:
I think that this trait should be subtrait of Format
and named FileFormat
. I think it plays better with mental model of things - FileFormat
is a Format
but with added information about file extensions. However, there is a problem - name FileFormat
is already reserved for an enum.
I am thinking about a better name, but maybe for backwards-compatibility sake it should be left the way it is (two unrelated traits bound only by structs that use them both).
Proposal out of top of my head: FormatWithExtensions
(sounds dumb, but something to start with).
I think I also should add something in README.md to inform users that that can define custom formats if needed more explicitly. |
6b6078b
to
e0df152
Compare
#[derive(Clone, Debug)] | ||
pub struct File<T> | ||
where | ||
T: FileSource, |
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 removed where clause on the struct as this is not recommended to have them either way.
4bebab6
to
3676320
Compare
@matthiasbeyer what do you think? |
🥳 |
This pull request introduces a
Format
trait (with its companionFileExtensions
to satisfy legacy behavior).Having such a trait allows users of the library to use custom/proprietary/less known formats with config-rs without the need to alter library code, which is not only a lengthy process, but might be impossible for some formats. What is more, having it could facilitate creating sources other than
File
that operate on the notion of Format, but are independent of a file concept (network sources).This pull request does not alter anything functionally speaking, but adds more genericness to already existing infrastructure. It makes the library more extensible.
I hope you like the idea!