-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
try to fix the declared with not given #2285
Conversation
try to fix #2275
case 1,2params
result
case 3params
result
|
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 is great, albeit a bit complicated. I don't have anything better to suggest, but consider some refactoring to make it a bit cleaner/better organized/.
- I think
include_not_dependent
should be called something else, what exactly does it do? exclude anything that is notgiven
(given is evaluated?) or anything that's inside anygiven
block? So maybeevaluate_given
or justgiven: true/false
? - Will nested given work? Let's add some tests/open some more bugs?
- This needs documentation, and CHANGELOG.
Thanks for your advice.
|
|
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.
Code looks good, some minor stuff below. Let's work on the documentation.
Regarding performance: this is only a problem when calling declared(params)
, right? So there's no regression during execution in most cases, so I am not worried. Let me know if I misunderstood.
I changed the structure of Thanks for the review, I will revise it soon. |
I revised the docs and modified the default value of |
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.
Looks great! Let's fix the English a little in README and this is good to go!
Great advise and I have changed these. |
I merged the change, thanks for contributing @zysend ! |
Thanks for your advise. |
No description provided.