Skip to content

config impl (pt.1) #1550

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

config impl (pt.1) #1550

wants to merge 13 commits into from

Conversation

Sk7Str1p3
Copy link
Contributor

@Sk7Str1p3 Sk7Str1p3 commented Mar 28, 2025

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

upd: i doubt functions 'custom' are really necessary

@Sk7Str1p3
Copy link
Contributor Author

@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
Copy link
Collaborator

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.

Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Sk7Str1p3
Copy link
Contributor Author

Sk7Str1p3 commented Mar 29, 2025

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
Copy link
Contributor Author

Sk7Str1p3 commented Mar 29, 2025

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
Copy link
Collaborator

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
Copy link
Collaborator

spenserblack commented Mar 31, 2025

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Sk7Str1p3 commented Mar 31, 2025

Speaking of key-value separator, check this out
IMG_20250331_081134_159.jpg

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

@spenserblack
Copy link
Collaborator

spenserblack commented Mar 31, 2025

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
Copy link
Contributor Author

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

@Sk7Str1p3
Copy link
Contributor Author

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

@spenserblack
Copy link
Collaborator

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

image
why does this freaky ':' appears?

@Sk7Str1p3
Copy link
Contributor Author

Sk7Str1p3 commented Apr 1, 2025

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

@Sk7Str1p3 Sk7Str1p3 requested a review from spenserblack May 7, 2025 19:07
@Sk7Str1p3 Sk7Str1p3 marked this pull request as ready for review May 7, 2025 19:07
@Sk7Str1p3
Copy link
Contributor Author

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

Copy link

@SirJiga SirJiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing interesting

@Sk7Str1p3
Copy link
Contributor Author

@spenserblack may I have a review?

@spenserblack
Copy link
Collaborator

@spenserblack may I have a review?

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

@Sk7Str1p3
Copy link
Contributor Author

Ah alright I'm sorry

@Sk7Str1p3
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@spenserblack
Copy link
Collaborator

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!

Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!

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

// THIS IS JUST A RAW, REALLY WIP CONFIG STRUCTURE
#[serde(default)]
pub disabled_fields: Vec<InfoType>,
// Lol is this really will turn into comment?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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;
}

Copy link
Contributor Author

@Sk7Str1p3 Sk7Str1p3 Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Sk7Str1p3 Sk7Str1p3 Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +19 to +24
#[serde(default)]
pub number_of_authors: usize,
#[serde(default)]
pub number_of_languages: usize,
#[serde(default)]
pub number_of_file_churns: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Sk7Str1p3 Sk7Str1p3 Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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


impl ConfigOptions {
pub fn read<P>(path: P) -> Self
// Is it even should panic?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(_).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Config file for theming/customization💄
5 participants