SerenityOS / serenity

The Serenity Operating System 🐞
https://serenityos.org
BSD 2-Clause "Simplified" License
30.68k stars 3.19k forks source link

LibCore: Better error handling for ConfigFile #9173

Open sppmacd opened 3 years ago

sppmacd commented 3 years ago

For example, we have:

    static NonnullRefPtr<ConfigFile> get_for_lib(const String& lib_name);
    static NonnullRefPtr<ConfigFile> get_for_app(const String& app_name);
    static NonnullRefPtr<ConfigFile> get_for_system(const String& app_name);
    static NonnullRefPtr<ConfigFile> open(const String& path);

which return NonnullRefPtr. The void reparse() just returns when file cannot be opened.

It seems that the only way to check if ConfigFile was properly opened is checking if it has any sections (but ConfigFile without sections is also a valid ConfigFile)

guerinoni commented 3 years ago

Mmm you are right but I'm trying to load an empty config file for irc client for example or other applications... In this case there is no crashes or other strange situation, the defaults values are used! Did you prefer RefPtr as return value?

sppmacd commented 3 years ago

Yes, I think it would be better if we allow applications handle errors on their own if they want. We can keep default values for simplicity (because that is often used).

We can have two versions of open functions: get_for_app which returns NonnullRefPtr (and just use default values as currently), and try_get_for_app which returns RefPtr that is null when open or parsing fails.

sin-ack commented 3 years ago

How about Result<RefPtr<ConfigFile>, String>? Rich errors are very nice.

sppmacd commented 3 years ago

How about Result<RefPtr<ConfigFile>, String>? Rich errors are very nice.

Hmm, why not?

sin-ack commented 3 years ago

Oops, meant Result<NonnullRefPtr<...>, String>.