Introduce `Format` trait #219

szarykott · 2021-08-01T11:36:03Z

This pull request introduces a Format trait (with its companion FileExtensions 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!

szarykott · 2021-08-01T11:40:51Z

It seems I will have to check Clippy on nightly.

matthiasbeyer

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... 😔

matthiasbeyer · 2021-08-20T07:21:02Z

examples/custom_format/main.rs

+// 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> {


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?

Good point with the impl, more flexible! Static lifetime should be enough, file extensions are quite ... static.

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.

matthiasbeyer · 2021-08-20T07:21:50Z

src/file/extension.rs

+/// 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.


nit: missing leading space

matthiasbeyer · 2021-08-20T07:23:12Z

src/file/format/mod.rs

@@ -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's a TODO: pub(crate) here, that could be resolved right away, right? 😄

Sure, I'll see what I can do

Required some changes in the example, but it is all pub (crate) now.

matthiasbeyer · 2021-08-20T07:24:49Z

src/file/mod.rs

-    pub fn from_str(s: &str, format: FileFormat) -> Self {
+impl<F> File<source::string::FileSourceString, F>
+where
+    F: Format + FileExtensions + 'static,


I wonder how the 'static bound behaves in production code then... 🤔

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.

szarykott · 2021-08-20T16:48:16Z

@matthiasbeyer I'll try to answer your concerns about production here.

Previously File source accepted only well-defined structs (so by definition none could use anything else). Those structs (one struct to be precise) all implement introduced traits, hence code should be backwards compatible - just see that I need not modify any test to make it compile and work.

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.

szarykott · 2021-10-17T07:54:21Z

Hey, @matthiasbeyer I pushed a lot of changes to make things green.

What do you think about this PR now?

matthiasbeyer

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)?

matthiasbeyer · 2021-10-17T16:43:15Z

src/format.rs

+    /// It also allows specifying optional URI of the source associated with format instance that can facilitate debugging.
+    fn parse(
+        &self,
+        uri: Option<&String>,


Is there any particular reason this is a &String and not a &str?

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 cloneing down there is probably a performance issue...

For now that's of course okay.

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.

I investigated a bit yesterday, will open an issue about that.

szarykott · 2021-10-17T20:55:27Z

@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?

matthiasbeyer · 2021-10-19T10:20:54Z

Awesome! If someone else takes a look, I'd love that (don't know who could, though 😄).

Looking forward to merging this!

szarykott · 2021-10-22T19:24:58Z

src/file/extension.rs

+/// 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 {


@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).

szarykott · 2021-10-22T19:39:57Z

I think I also should add something in README.md to inform users that that can define custom formats if needed more explicitly.

szarykott · 2021-10-25T08:03:27Z

src/file/mod.rs

 #[derive(Clone, Debug)]
-pub struct File<T>
-where
-    T: FileSource,


I removed where clause on the struct as this is not recommended to have them either way.

szarykott · 2021-11-04T21:54:37Z

@matthiasbeyer what do you think?

matthiasbeyer · 2021-11-08T06:33:01Z

🥳

matthiasbeyer requested changes Aug 20, 2021

View reviewed changes

szarykott mentioned this pull request Aug 23, 2021

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

Merged

szarykott force-pushed the trait_format branch 2 times, most recently from fad912e to c1002cb Compare September 30, 2021 20:43

matthiasbeyer added the hacktoberfest-accepted label Oct 12, 2021

szarykott force-pushed the trait_format branch 2 times, most recently from 01bea1c to 9e5e2f8 Compare October 17, 2021 07:44

matthiasbeyer approved these changes Oct 17, 2021

View reviewed changes

szarykott commented Oct 22, 2021

View reviewed changes

szarykott added 11 commits October 23, 2021 16:58

Add Format trait

a78fc37

Implement Format for FileFormat

141ec79

Add FileExtensions trait

63c16af

Implement FileExtensions trait for FileFormat

0cf715f

Make File generic over Format and FileExtensions

abe7f76

Satisfy clippy's type_complexity lint

4775107

Add documentation to file related types

bb23458

Add custom_format example

6fa1b27

Change FileExtensions signature

583a477

Apply clippy lints and format

5ba9013

Add information to changelog and README

e0df152

szarykott force-pushed the trait_format branch from 6b6078b to e0df152 Compare October 23, 2021 14:59

Change FileExtensions (rename, make subtrait)

b09d57a

szarykott commented Oct 25, 2021

View reviewed changes

Fix reqwest version to make it work on rust 1.46

3676320

szarykott force-pushed the trait_format branch from 4bebab6 to 3676320 Compare October 27, 2021 20:11

szarykott requested a review from matthiasbeyer November 7, 2021 21:12

matthiasbeyer approved these changes Nov 8, 2021

View reviewed changes

matthiasbeyer merged commit 6de19be into rust-cli:master Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `Format` trait #219

Introduce `Format` trait #219

szarykott commented Aug 1, 2021

szarykott commented Aug 1, 2021

matthiasbeyer left a comment

matthiasbeyer Aug 20, 2021

szarykott Aug 20, 2021

szarykott Sep 30, 2021

matthiasbeyer Aug 20, 2021

matthiasbeyer Aug 20, 2021

szarykott Aug 20, 2021

szarykott Sep 30, 2021

matthiasbeyer Aug 20, 2021

szarykott Aug 20, 2021 •

edited

Loading

szarykott commented Aug 20, 2021 •

edited

Loading

szarykott commented Oct 17, 2021

matthiasbeyer left a comment

matthiasbeyer Oct 17, 2021

matthiasbeyer Oct 17, 2021

szarykott Oct 17, 2021

matthiasbeyer Oct 18, 2021

szarykott commented Oct 17, 2021

matthiasbeyer commented Oct 19, 2021

szarykott Oct 22, 2021

szarykott commented Oct 22, 2021

szarykott Oct 25, 2021

szarykott commented Nov 4, 2021

matthiasbeyer commented Nov 8, 2021

Introduce Format trait #219

Introduce Format trait #219

Conversation

szarykott commented Aug 1, 2021

szarykott commented Aug 1, 2021

matthiasbeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szarykott Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

szarykott commented Aug 20, 2021 • edited Loading

szarykott commented Oct 17, 2021

matthiasbeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szarykott commented Oct 17, 2021

matthiasbeyer commented Oct 19, 2021

Choose a reason for hiding this comment

szarykott commented Oct 22, 2021

Choose a reason for hiding this comment

szarykott commented Nov 4, 2021

matthiasbeyer commented Nov 8, 2021

Introduce `Format` trait #219

Introduce `Format` trait #219

szarykott Aug 20, 2021 •

edited

Loading

szarykott commented Aug 20, 2021 •

edited

Loading