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?

[spenserblack]

These probably don't need to be in JSON outputs (or other machine-readable outputs).

[Sk7Str1p3]

I could not find how should I remove it from output as rustc complains about "missing field"

[spenserblack]

I think serde has an attribute to tell it to ignore when converting to JSON, YAML, etc.

[Sk7Str1p3]

It is (serde(skip)) but this will prevent from writing default value in config

[Sk7Str1p3]

@spenserblack I need some help with Tera for per-language configuration field. It's very first time working with generators and for now I have no clue how could I implement those. Maybe there're already some references could help me? If so, may you show them or at least send link to documentation? thx

[spenserblack]

Maybe there're already some references could help me? If so, may you show them or at least send link to documentation?

These are probably what you need:

• https://keats.github.io/tera/docs/#control-structures
• https://keats.github.io/tera/docs/#built-ins

Could you tell me what you're planning to write that involves writing with tera instead of Rust? I don't think you need to unless you're iterating over language entries.

[Sk7Str1p3]

Sorry for late answer

I don't think you need to unless you're iterating over language entries.

That's exactly what I planned to implement. As you could see in my "todo", it contains custom image/ascii logo fields on every language. I don't want to write 100+ identical structures and implementation. I investigate on documentation and onefetch source code so possibly we will have something ready to merge really soon

[Sk7Str1p3]

Lol I think i implemented the most useless feature imaginable. Shall I revert commit or better keep it?

[Sk7Str1p3]

@spenserblack I really need your review on tests. I never worked with them and don't really understand how should I edit them according to changes. Also I feel like I made many shitcode

[spenserblack]

That's exactly what I planned to implement. As you could see in my "todo", it contains custom image/ascii logo fields on every language.

I think it would be wiser to consider this a per-project configuration than a per-language configuration. So, for example, instead of configuring your favorite Rust logo, you would configure your logo for the onefetch repository. But, regardless, we haven't discussed this sort of configuration AFAIK, so I think it's best to save that for later and focus on the basic stuff. You don't need to add all the coolest features all at once. You can add just the simplest config that supports really basic options, and once that's done, we can say "cool, now what's next?"

[spenserblack]

I really need your review on tests. I never worked with them and don't really understand how should I edit them according to changes.

Looks like the CI can't run until merge conflicts on Cargo.toml and Cargo.lock are resolved. Think you could try resolving those? Merge conflicts are kind of a pain with lock files -- I would recommend using git checkout --theirs -- Cargo.lock when resolving merge conflicts, then let cargo update it by calling cargo check.

At a very quick glance I didn't see anything too bad. Were you concerned about the snapshot test? Or some other tests?

[hasecilu]

A minor bug on current state: when running onefetch --nerd-fonts if the config file contains nerd_fonts = false (or if setting is not there) the flag does not overrides the setting from the config file, I think it should.

[Sk7Str1p3]

It's not a bug but an unimplemented feature, I will workaround it a bit later

[spenserblack]

@Sk7Str1p3 Just want to say that the "todo" in your latest changes to the PR body looks perfect 👍

[Sk7Str1p3]

@spenserblack i consider reverting all my commits refactoring this pull request to onefetch reimplementation. I found current code non-extendable and hard to edit, especially BuildInfo struct. Also some stuff could be optimised.

By the way, i found out new config format, RON. It could be also candidate to be config format, as more native to rust?

Now, I need you to confirm or deny. Waiting for answer...

[spenserblack]

By the way, i found out new config format, RON. It could be also candidate to be config format, as more native to rust?

I wouldn't really use RON unless you can expect users to almost entirely be Rust developers. I think TOML is a happy medium between "easy to use" and "familiar to the Rust ecosystem."

I found current code non-extendable and hard to edit, especially BuildInfo struct. Also some stuff could be optimised.

For now, I think just focus on the configuration options that are already easy to work with (so the things that are also configurable via CLI). Adding configuration for more difficult things can then be worked on individually, instead of stressing on how to add a bunch of difficult config options at once.

But if you have ideas for optimization, feel free to share 🙂