git-graph icon indicating copy to clipboard operation
git-graph copied to clipboard

Solves #65 - Uses git-graph.toml to parse commits message format

Open KarlHeitmann opened this issue 2 years ago • 4 comments

Intro

This solves #65 , it is a proof of concept and I think needs some iteration. To test it, copy the sample below into the file .git/git-graph.toml (you can put whatever you want on format="whateveryouwant" following the rules on cargo run -- --help section --format)

model="simple"
format="%H"

Questions

  • Can I recycle the file that is already being used here to get the model name, and add the "format" key-value pair to get the format by config file? If we want to use the same file to store all default custom settings permanently, we need to wrap inside an Option the model and format attributes for RepoSettings and CommitFormatToml, as you can see on the second commit. If we don't wrap these attributes inside an Option, the program will crash if the config file has only model="simple", or has only format="blabla" key-value pair.

  • The rest is implementation details, should I move the new code that tries to parse the config file from src/main.rs to src/config.rs? Is there a better way to not have so much nested matches? Every feedback is welcome.

KarlHeitmann avatar Jun 16 '23 04:06 KarlHeitmann

Another solution that will avoid modifying RepoSettings and CommitFormatToml structs is to simply use the error thrown upon parsing the toml file, and fallback model and branch to their default values. This solution can be more elegant.

BTW, if you have better name for CommitFormatToml or a better place to put it just tell me and I'll fix it in the next iteration.

KarlHeitmann avatar Jun 16 '23 04:06 KarlHeitmann

Sorry, a bit short in time at the moment. So some more patience required until I can take a deeper look...

Could you please check if the cargo fmt warnings are relatd to your edits (may also be changes to fmt affecting old code)?

mlange-42 avatar Jun 19 '23 21:06 mlange-42

Don't worry, I am not in a hurry.

EDIT: Yes, I've run cargo fmt and it automatically linted the code and now it is passing.

~I ran the cargo fmt command but it did not complain. I noted that the rustc version used by the linter is 1.70, so I switched to rust nightly. I also ran this command:~

❯ cargo fmt --all -- --check

~But I've got no complains at all. I didn't get any warnings neither while compiling. And tried to see if there is a dot file with the command that is run by github action linter and did not found any complain. I don't know how to reproduce the action.~

KarlHeitmann avatar Jun 21 '23 02:06 KarlHeitmann

I would prefer to have both settings in the same file, and to use Option to make them optional.

To make things consistent, setting the format permanently should work like setting the model permanently. This would also help to avoid that users are required to edit the file, and make the example file from #70 unnecassary.

Given that more settings could be permanent, maybe a different CLI syntax for permanent settings would be better.

Now, it works like this:

git-graph --model simple  # non-permanent
git-graph model simple    # permanent

Maybe something like this would be better for permanent settings:

git-graph set --model simple

mlange-42 avatar Jul 06 '23 08:07 mlange-42