-
Notifications
You must be signed in to change notification settings - Fork 84
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
add config for zero assertions switcher in config and cli args #404
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
==========================================
- Coverage 75.08% 74.86% -0.22%
==========================================
Files 52 52
Lines 2797 2809 +12
Branches 267 270 +3
==========================================
+ Hits 2100 2103 +3
- Misses 530 536 +6
- Partials 167 170 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@@ -8,6 +8,11 @@ | |||
[tag parent] | |||
(alter-var-root #'hierarchy derive tag parent)) | |||
|
|||
(defn underive! | |||
"Add a parent/child relationship to kaocha's keyword hierarchy." |
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.
This removes a parent/child relationship, right?
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.
yes
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.
Then I'd suggest:
"Add a parent/child relationship to kaocha's keyword hierarchy." | |
"Remove a parent/child relationship from Kaocha's keyword hierarchy." |
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.
Are you planning to make this change?
Since we might want to make other warnings configurable, I think we should either:
I didn't closely review the rest of the code (although it looks good on first glance) since I think it will have to change if you take one of my suggestions. |
I agree with Alys, we have a number of warnings that could be configurable. I would rather not introduce a new top level key for each. Instead we should introduce a pattern that people can easily extend when they want to make other warnings configurable. |
I'm leaning toward the latter option (the |
42db155
to
7ecc8dd
Compare
7ecc8dd
to
c178ed0
Compare
I have implemented the config argument as you suggested. On the other hand, in this case, do I still need to implement |
@humorless Yes, that's tricky. I'm not sure there's a great way to do key-value command line arguments. (I found some discussion here). Maybe we could use an approach similar to Usually I'd expect people would modify it in |
I also tried to provide a solution to #211 in this MR.
or
in By the way, I somewhat prefer to delay the command line argument design for later PR. I really think that is there really a need to control kaocha to this level with command line arguments? |
Yes, that makes sense to me. |
A possible refactoring would be a function that outputs using the correct level based on the warning. For example, |
This is for #162
kaocha's default behavior will let the tests fail if there are no
is
in assertion.With this MR, we now support a new config/cli args to switch off the
zero-assertions
Example of
tests.edn
This MR can also fix #211
Example of
tests.edn
;; => Results