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

pattern: FOR pattern correctness and unit test #40

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

Conversation

PXke
Copy link

@PXke PXke commented Oct 25, 2016

  • FIX Saves previous data if an iteration has been done (closes KeyError: 'previous_data' #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

* 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")])])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦄

@PXke
Copy link
Author

PXke commented Nov 1, 2016

Dear @gas0189 can you try with this branch to see if it works correctly ?
@kaplun Can I get a review please :) ?

@@ -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
Copy link
Member

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:
Copy link
Member

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 ?

Copy link
Member

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

Copy link
Author

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

@PXke
Copy link
Author

PXke commented Nov 2, 2016

@david-caro, thanks for the review, I will take a look soon to improve the points you have pointed out.

Best,

@fangkyo
Copy link

fangkyo commented Sep 11, 2017

@PXke, is there any plan to check in your code? The FOR doesn't work without your FIX in version 2.1.2.

Thanks,

@ghost ghost assigned PXke Jun 28, 2018
@ghost ghost added the Review: WIP label Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeyError: 'previous_data'
4 participants