improve error handling for loading config file

[ivinjabraham]

Prerequisite

This change assume we can change App::new() to initialize logging earlier, since currently it is only initialized after Config is built as seen here:

    pub fn new() -> App {
        let config: Config = Config::build();
        config.create_dirs();

        ...

        // Initialize the logger before the app starts
        Logger::init_log_file(&config);
        Logger::info("patch-hub started");
        logging::garbage_collector::collect_garbage(&config);

And it seems #99 should allow us to do so.

Overview of changes

Previously, errors when failing to read or parse the config. file were not logged. Errors in failing to read was also silently mitigated by defaulting to an empty string, which could lead to more problems down the road.

Users are still not notified directly (they will have to check the logs) when this happens, and I think that might be necessary?

[davidbtadokoro]

Hi @ivinjabraham, and thank you so much for this first contribution! I extend my deepest apologies for the massive delay in reviewing it...

My take on the motivation

I soundly agree with the motivation behind the PR, as indeed, we are careless in handling errors when reading and parsing the configurations, especially when the config file "exists" (when it doesn't, it isn't an error, just that patch-hub config file hasn't been initialized).

My take on the change itself

The implementation seems correct theoretically, but I couldn't actually test due to the fact (that you've pointed out) that the Logger is initialized after resolving the Config, and that the former depends on the latter logs_path field.

In all trueness, this coupling is a bad smell, and now (considering #99, also) I do think that this logs_path should be independent of Config, even though it can be considered a configuration...

I think that we will need to wait for the merge of #99, along with some adaptations, to make this PR feasible, and I imagine this is why you've opened it as a draft

Only change requested

As I've said, your work is marvelous, from the motivation to the implementation and also in the commit message description (kudos for this!). However, the commit message subject pattern used isn't the one we use here in patch-hub, which is the Semantic Commit Messages.

I reckon you've gone with the "descriptive path pattern" as this is the one we use in the parent project kworkflow and the fact that patch-hub lacks dedicated contribution guidelines. This speaks more about the infancy of patch-hub than a mistake on your side.

Conclusion

Thank you so much for this PR (and the other ones you've opened!), and will ping you here when we are ready to move forward on it!

[ivinjabraham]

Thank you for the feedback! I feel a bit embarrassed about the seven or so other commits that all had the path pattern in the commit messages. I will amend those messages ASAP.

I do think that this logs_path should be independent of Config, even though it can be considered a configuration...

I also think that logging options (including logs_path and max_log_age) and some other fields like cache_dir isn't something the end user is usually concerned with. Since these are a kind of meta-configuration and don't impact the app's functionality, perhaps we could separate it from the Config struct?

[davidbtadokoro]

Thank you for the feedback! I feel a bit embarrassed about the seven or so other commits that all had the path pattern in the commit messages. I will amend those messages ASAP.

No worries! You did a great job and this was somewhat an unwritten rule.

I also think that logging options (including logs_path and max_log_age) and some other fields like cache_dir isn't something the end user is usually concerned with. Since these are a kind of meta-configuration and don't impact the app's functionality, perhaps we could separate it from the Config struct?

That is the idea, but, as I realized and mentioned in the recent review of PR #100, this challenge is a little deeper than I first thought. TLDR, I think that initializing Config first before anything else except maybe the Cli parser, may be the approach with the best tradeoffs, as, like in the example of the logger, configurations may be needed at the inception of patch-hub. Of course, we would need to extract it from App (which would become a parameter of App::new()).

So maybe we should make an exception with config and handle its errors using eprintln if necessary. This PR wouldn't suffer from this, I think.

[davidbtadokoro]

Hi @ivinjabraham, thanks for the update! I like that you've adapted the PR with our discussion. There is a "my bad" on my end, where I remembered that eprintln! only works on patch-hub if we redirect the stderr (&2) to a file or something else, because when ratatui is in possesion of the terminal it "swallows" any and all eprintln!.

This doesn't concern you as, from what we talked in #100, we are locked in using eprintln! in the initialization of Config. In this sense, change merged into the unstable branch 👍

Quick note: I've noticed that this PR is from your unstable branch. By now, I know already that you get that we need to create a new branch for each PR, so just giving you this head ups for you to not mess your unstable branch 🙃

[ivinjabraham]

when ratatui is in possesion of the terminal it "swallows" any and all eprintln!.

Ah I could have sworn I had seen this documented elsewhere in the project. I couldn't find it earlier so I thought I was hallucinating and made the change anyway.

Quick note: I've noticed that this PR is from your unstable branch. By now, I know already that you get that we need to create a new branch for each PR, so just giving you this head ups for you to not mess your unstable branch 🙃

Hehe, I was hoping this silly mistake would go unnoticed! I forgot to checkout out to a new branch when working on this patch, and every PR I had hence had to be checked out manually from head/upstream.