-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: add config file support #152
Conversation
cc8860c
to
dd82840
Compare
dd82840
to
c754d56
Compare
1e88799
to
a99b203
Compare
a99b203
to
a50da0d
Compare
@@ -0,0 +1,12 @@ | |||
'$schema' = "https://awslabs.github.io/duvet/config/v0.4.0.json" |
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.
what's going on with this syntax? '$schema'
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.
There's three reasons for it:
- I want config files to be versioned so we can make changes to the format over time and preserve support for older ones. I could have gone with
version
instead but, then that leads to the second point. - This syntax is taken from JSON schema, where you can have your editor validate that your schema conforms to what we define. By using
$schema
, editors automatically download that URL and perform validation (see https://taplo.tamasfe.dev/configuration/using-schemas.html). It's not the nicest thing to look at... But it makes it so we don't have to register the schema anywhere and can just self host. - Unfortunately, toml doesn't allow keys to start with a
$
... So you have to quote it. Which is annoying. But I think the two previous points justify it?
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.
Thanks for the explanation!
"type": "string" | ||
} | ||
} | ||
} |
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.
should these have a newline at the end?
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.
It should be ok without it. These are auto-generated and really only consumed by programs. But I can add one of you feel strongly about it.
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 dont feel very strongly, its more just the way GitHub highlights it that bothers me :-)
".duvet/requirements/**/*.toml", | ||
".duvet/todos/**/*.toml", | ||
".duvet/exceptions/**/*.toml", |
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.
any docs this should be mentioned in so the user knows they don't need to add these?
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.
Yeah I will add that in the guide
@@ -139,17 +193,17 @@ fn extract_section(section: &Section) -> (&Section, Vec<Feature>) { | |||
|
|||
if KEY_WORDS_SET.is_match(line) { | |||
for (key_word, level) in KEY_WORDS.iter() { | |||
for occurance in key_word.find_iter(line) { | |||
for occurrence in key_word.find_iter(line) { |
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.
guess we need to add the typo check to CI
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.
Haha yep. I added it to my editor and found a few issues 😁
Co-authored-by: Wesley Rosenblum <[email protected]>
Description of changes:
This change adds support for config files for configuring
duvet
, rather than CLI arguments. This should standardize how customers set up their repositories/reports and avoid having one-off scripts everywhere:The config location is
.duvet/config.toml
. All of the artifacts andtoml
files are stored alongside in.duvet/
. I've included a config file for the duvet repo itself:With a config file, generating a report no longer requires any arguments:
$ duvet report
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.