-
Notifications
You must be signed in to change notification settings - Fork 46
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
pattern: FOR pattern correctness and unit test #40
base: master
Are you sure you want to change the base?
Conversation
* FIX Saves previous data if an iteration has been done (closes inveniosoftware-contrib#39). * FIX Calls the correct function when the list of item to iterate is generated. * FIX Handles special case where the list of item to iterate is empty. * NEW Adds unit tests covering 100% of FOR pattern. Signed-off-by: Guillaume Lastecoueres <px9e@gmx.fr>
|
||
# We check normal workflow appending 50 times pony to a list. | ||
# This test will check multiple object processing with for loop. | ||
we.setWorkflow([cf.FOR(range(0, 50), "_loops", [a("pony")])]) |
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.
🦄
@@ -257,8 +255,6 @@ def FOR(get_list_function, setter, branch, cache_data=False, order="ASC"): | |||
:param setter: function to call in order to save the current item of the |
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.
The usage that is done of this on line 267 is not documented here right? It seems as if you could pass a string as setter
and it will generate the function with _setter
(order == "DSC" and step_value > -1) | ||
if currently_within_list_bounds: | ||
# Store current data for ourselves | ||
eng.extra_data["_Iterators"][step]["current_data"] = \ | ||
my_list_to_process[step_value] | ||
list_to_process[step_value] | ||
# Store for the user | ||
if setter: |
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.
When is bool(setter) == False
?
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 know that it's not part of your patch, just trying to understand it, I don't mean that you should change this in this pr, but will help me understand the code, and thus this pr, and hopefully help you 😉)
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.
Sure, if you have a question reading the code it means it is not clear enough and should be rework/commented, so no problem, it is a great addition.
Best
@david-caro, thanks for the review, I will take a look soon to improve the points you have pointed out. Best, |
@PXke, is there any plan to check in your code? The FOR doesn't work without your FIX in version 2.1.2. Thanks, |
generated.
empty.
Signed-off-by: Guillaume Lastecoueres px9e@gmx.fr