Skip to content
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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KarlHeitmann
Copy link

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

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.

@mlange-42
Copy link
Owner

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)?

@KarlHeitmann
Copy link
Author

KarlHeitmann commented Jun 21, 2023

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.

@mlange-42
Copy link
Owner

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

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.

2 participants