config impl (pt.1)

[Sk7Str1p3]

Goals

Implements advanced customisation with config file in toml format. This pull request (pt.1) aims to implement just basic configuration, and pt.2 will go much deeper with it.
Resolves #745

Todo

Target	Progress
Allow user define existing cli opts in config file	setup config parser create basic config structure override config parametrs with values from CLI
Implement new cli/config options	modules order (requires major changes in logic) custom functions
Advanced	per-language configuration (ascii/pic logo, color, etc..) per-project (see Config file for theming/customization💄 #745 (comment))

upd: i doubt functions 'custom' are really necessary

[Sk7Str1p3]

@spenserblack I'm sorry for pinging you that many times... I opened new PR

Current problem is config is not spawning at $XDG_CONFIG_HOME, but it spawns folder. I am not able to check how does config parsed because is not created. Waiting for your answer, thx

[spenserblack]

I'm sorry for pinging you that many times

No worries.

I am not able to check how does config parsed because is not created.

Since you're using serde, you should be able to read the config from a string. Or a reader type that uses a string. I'd honestly use that for testing. You can use include_str!("path/to/test-config.toml"), load the configuration from the TOML string, and test its values.

Since the CLI type implements Parser, you can also test the CLI by passing one of the iterator types (simplest being an array of strings) to .try_parse_from, which will then let you test how the CLI and config resolve together.

I'd recommend putting the tests and test config file into tests/ to organize things, because we probably don't want random .toml files in the source code.

[spenserblack]

See review comment. I think it should help you figure out why the config file isn't being created.

[Sk7Str1p3]

i think i should write a todo with what are my plans on customisation with config

config sample:

separator = " -> "
number-separator = comma

# this should support custom ascii and images
# if images not supported will fallback to custom ascii if provided and standard if not
[logo] 
ruby = ./ruby.png
c = ./c.txt
rust = { image = ./rust.png, ascii = ./rust.txt }

# like --disabled-field, but lets you change order
modules = """
  name
  break

  project
  created
  authors
  break

  head
  last-change
  pending
  churn
  lines-of-code
  break

  size
"""

# detailed settings on module
[name]
format = -----$name|$version-----

[Sk7Str1p3]

Nah, we won't get any good customization if we just parse config as CLI arguments. I reverted all commits

I am looking forward to CrabFetch approach. All customisation made by configuration file, and CLI just lets you some minimal settings.
We can also do things like FastFetch does. It has enormous amount of CLI arguments and uses values in config as arguments. I do NOT like that.

@spenserblack what's your opinion on that?

[spenserblack]

Nah, we won't get any good customization if we just parse config as CLI arguments.

Could you explain this a bit more? You hint at overriding logos per-language in #1550 (comment), which wouldn't work if config and CLI are identical. Is that the main issue, or are there others?

I am looking forward to CrabFetch approach. All customisation made by configuration file, and CLI just lets you some minimal settings.
We can also do things like FastFetch does. It has enormous amount of CLI arguments and uses values in config as arguments. I do NOT like that.

Each repository is different, and can require different options. For example, if a repository has a logo, show that with -i. In many repositories, the user would want to use -T to get stats that better represent the "real" dominant language. Frankly, if I had to write a config file to set these, a config file that I may never use again (likely if I'm not a maintainer of the repo I'm analyzing), I would find it a bit annoying. IMO, simply allowing users to store and not repeat some CLI arguments is a big win. But I'm still of the opinion that the design should be CLI first, config file secondary.

[spenserblack]

OK, I've just reread the idea that was documented in #745. I had kind of forgotten the original intent of that issue 😅

I've noticed that the original issue body was mainly focusing on display configuration options. This makes sense to me, since those are the types of things that I'd likely only want to set once, not per-repo. Also, some options would be kind of awkward to set via the CLI, like if I was trying to set Nerd Font icons for the info lines.

So, yeah, I'm backtracking on the config file being a 1:1 with the CLI. Instead, we should focus on a limited set of options that we're very confident would only be set once, regardless of repo. And also some options that may work best in a config file instead of as CLI arguments.

@o2sh Thoughts?

[Sk7Str1p3]

Instead, we should focus on a limited set of options that we're very confident would only be set once, regardless of repo

Those could be key-value separator, number separator, nerd-fonts switch and natural colors (use colors set by terminal) switch (to be implemented).

Im working on those and possibly finish in just few hours.

[Sk7Str1p3]

Speaking of key-value separator, check this out

This confusing me, why that would happen? Why I could not use -> as separator?

[spenserblack]

This confusing me, why that would happen? Why I could not use -> as separator?

That's going to be caused by clap, the library for argument parsing. It's reading the next argument after --separator and seeing ->. I believe from there it's checking that the next argument starts with - (it does), and interpreting that as another CLI switch/option instead of as the value passed for --separator.

There's perhaps an option that would force clap to take whatever argument is next, regardless of format.

[Sk7Str1p3]

AYO I MADE BASE FOR CONFIG!!1!
currently slowly creating commits

[Sk7Str1p3]

@spenserblack I got one annoying error with config file - values MUST be set there, otherwise onefetch is crashing

[spenserblack]

@spenserblack I got one annoying error with config file - values MUST be set there, otherwise onefetch is crashing

I think the easiest thing to do would be to just wrap them all in Option to make them nullable.

Also, ideally, let's not remove options from the CLI. Here's roughly how shared options should be resolved.

let opt = cli.opt.or(config.opt).unwrap_or(default_opt);

Check out the huge amount of helper methods defined on Option<T>.

[Sk7Str1p3]

Also, ideally, let's not remove options from the CLI. Here's roughly how shared options should be resolved.

i dont really think someone would like to override such things like separators from CLI

by the way, check latest commit, i did wrap values on Option, but code seems kinda idiotic imo

[Sk7Str1p3]

why does this freaky ':' appears?

[Sk7Str1p3]

@spenserblack i could not really find why ":" is hardcoded as separator, do you have any ideas?

[Sk7Str1p3]

@o2sh ready to merge (just will have to recommit in prettier way), and I would like to get some advices on my code

[SirJiga]

Nothing interesting

[Sk7Str1p3]

@spenserblack may I have a review?

[spenserblack]

@spenserblack may I have a review?

My availability is limited right now, but don't worry, your PRs are still on my radar 🙂

[Sk7Str1p3]

Ah alright I'm sorry

[Sk7Str1p3]

By the way, speaking of optimisations, I noticed there are many clone() methods are used (especially then cli_options are being parsed). I think we can easily replace them with references as they have same lifetime.
I would start working on it after this gets merged

[Sk7Str1p3]

Oh it's already 2 months since I opened PR lol

[spenserblack]

I think we can easily replace them with references as they have same lifetime.
I would start working on it after this gets merged

Sounds good! Yes, in a separate PR please to make the changes smaller and easier to review. Thanks!

[spenserblack]

Almost there!

Besides my review comments, there are also merge conflicts that would block this PR.

[spenserblack]

// for normal comments, /// for documentation comments. More specifically:

pub mod foo {
    //! This documents the thing that this comment is *inside.* So this
    //! documents `mod foo`
    /// This documents the thing *underneath* the comment, so this documents
    /// `struct Foo`
    pub struct Foo;
}

[Sk7Str1p3]

I wanted to write comments directly to config file and was testing which type would be written. I couldn't find any info about it (maybe I'm bad searcher lol)

[spenserblack]

The Rust book is a pretty good resource -- it's what I used to learn Rust.
Here's the section on doc comments: https://doc.rust-lang.org/book/ch14-02-publishing-to-crates-io.html#making-useful-documentation-comments

[Sk7Str1p3]

I know about rustbook, this is how I learned rust. And I also acknowledged about different comment types

But the question was, how do I turn /// doc into comment in .toml file.
So this

struct foo {
    /// this is commented line
    #[serde(default = 54)]
    bar: i32
}

turns into this

# this is a commented line
bar = 54

[spenserblack]

usize::default() is 0, which I don't think we want. There should be a serde option to specify what the default value should be, instead of falling back to Default::default(), which I believe this is doing.

[Sk7Str1p3]

Wait, wait, wait, doesn't serde fallback to values set in struct's Default trait implementation?

[Sk7Str1p3]

hm, yes, it seems to fallback to Default::default().

I think i shouldn't switch from Option<T> to serde fallbacks.

[spenserblack]

Is it even should panic?

Probably not 😆 At least not here.
This can return a Result<Self, _>, and then somewhere else we can decide what to do when it returns an Err(_).

[Sk7Str1p3]

oh, i made wrong question. Is it even should panic, or rather in place return Self::default()? this was the quiestion, im sorry for confusing