-
Notifications
You must be signed in to change notification settings - Fork 288
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
base: main
Are you sure you want to change the base?
config impl (pt.1) #1550
Conversation
@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 |
No worries.
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 Since the CLI type implements I'd recommend putting the tests and test config file into |
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.
See review comment. I think it should help you figure out why the config file isn't being created.
i think i should write a todo with what are my plans on customisation with config config sample:
|
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. @spenserblack what's your opinion on that? |
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?
Each repository is different, and can require different options. For example, if a repository has a logo, show that with |
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? |
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. |
That's going to be caused by clap, the library for argument parsing. It's reading the next argument after There's perhaps an option that would force clap to take whatever argument is next, regardless of format. |
AYO I MADE BASE FOR CONFIG!!1! |
@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 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 |
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 |
@spenserblack i could not really find why ":" is hardcoded as separator, do you have any ideas? |
@o2sh ready to merge (just will have to recommit in prettier way), and I would like to get some advices on my code |
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.
Nothing interesting
@spenserblack may I have a review? |
My availability is limited right now, but don't worry, your PRs are still on my radar 🙂 |
Ah alright I'm sorry |
By the way, speaking of optimisations, I noticed there are many |
Oh it's already 2 months since I opened PR lol |
Sounds good! Yes, in a separate PR please to make the changes smaller and easier to review. Thanks! |
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.
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? |
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.
//
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;
}
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 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)
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.
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
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 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
#[serde(default)] | ||
pub number_of_authors: usize, | ||
#[serde(default)] | ||
pub number_of_languages: usize, | ||
#[serde(default)] | ||
pub number_of_file_churns: usize, |
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.
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.
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.
Wait, wait, wait, doesn't serde fallback to values set in struct's Default
trait implementation?
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.
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? |
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 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(_)
.
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.
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
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
(requires major changes in logic)
(ascii/pic logo, color, etc..)
upd: i doubt functions 'custom' are really necessary