[enhancement]evaluating more than 1 condition fixes #277

[nepython]

This is a improvement which fixes issue #277(A feature request to be able to apply more than 1 condition in a go).

[nepython]

@dirk-thomas , can you please review the changes.

[nepython]

I will make the recommended changes.

Please also add unit tests for the newly supported cases as well as make sure that all tests pass.

Regarding that, The tests failed as I have exceeded the number of maximum commits in pull request allowed by flake. I think, I won't be able to squash my commits as I had not cloned them priorily. If you check the commit prior to last in this pr, it passed the tests. So a request, please squash and merge them after I implement your recommendations.

[dirk-thomas]

The tests failed as I have exceeded the number of maximum commits in pull request allowed by flake.

I don't think such a limitation exists.

Please re-read the error message from the CI build (trailing whitespace) and commit a fix for it to the branch of your PR.

I won't be able to squash my commits as I had not cloned them priorily.

You can easily clone your forked repo, switch to the branch of this PR, and apply any kind of change necessary - if need be even a squash of commit (which I highly doubt is necessary).

please squash and merge them after I implement your recommendations.

The PR will not be merged until it passes CI. Please address the above comments and CI will pass.

[nepython]

@dirk-thomas , you were correct.

This line contains trailing white spaces which is flagged by the linter.

Thanks for pointing it out. :relaxed: I have applied all your suggested changes, can you please review them.

[dirk-thomas]

Please also add unit tests for the newly supported cases

Still pending.

[nepython]

@dirk-thomas , the unit tests have been implemented. I have run the code numerous times taking into account many edge cases, the results were as desired. Also, I have taken into account precedence order in logical and and or . So, can you please review the changes.

[nepython]

@dirk-thomas, can you please review it?

@dirk-thomas, Gentle Reminder :smile:

[nepython]

@dirk-thomas Gentle Reminder :smile:

[nepython]

@dirk-thomas, can you please review the code. It's been almost a week.

[dirk-thomas]

@nepython I will review the pull request as soon as I find time for it. Poking me four times won't generate more time on my side unfortunately.

[dirk-thomas]

I took another look at the patch and the logic with this loop and additional recursion doesn't make any sense to me. The function already does check for the logical operators and and or below and does invoke itself recursively. I don't see why all of that should be duplicated.

It should be completely sufficient to reduce this patch to two simple changes:

• change the existing assert to assert len(parse_results) >= 3 and len(parse_results) % 2 == 1 to allow parser results with odd number of items greater 3 and
• modify the second argument for the recursive invocation to parse_results[2] if len(parse_results) == 3 else parse_results[2:].

[nepython]

@dirk-thomas, thank you for the review :smiley:

The function already does check for the logical operators and and or below and does invoke itself recursively. I don't see why all of that should be duplicated.

I had implemented these to account for short-circuiting which is quite useful when evaluating logical expressions.

if (_evaluate(parse_list, context)):   -->Check whether the condition being evaluated is True
    if (parse_results[i + 2] == 'and'):
        parse_list = parse_results[i + 1:i + 4]
    if (parse_results[i + 2] == 'or'):     ->and if the next logical operator is 'or', 
        return True                          no need to evaluate further
else:                                  -->If the condition being evaluated is False
    if (parse_results[i + 2] == 'and'):    ->and if the next logical operator is 'and', 
        return False                          no need to evaluate further
    if (parse_results[i + 2] == 'or'):
        parse_list = parse_results[i + 1:i + 4]

Note:Above, condition refers to a list containing three arguements like [['bar', '<', 'baz'], 'and', ['foo', '<', 'bar']]

If you don't find them useful, please comment it and I will make the changes as per your suggestion.

Though, after applying your second suggested change(which I assume makes the loop go on until all conditions have been evaluated or some condition evaluates to False) there will be a very small problem, suppose the condition being evaluated, evaluates to False and the next logical operator is or. In this case, the recursive loop will stop as the condition evaluated toFalse, though logically it was supposed to go on.

[dirk-thomas]

I had implemented these to account for short-circuiting which is quite useful when evaluating logical expressions.

What do you mean with this? Why is it being useful? Please try to be more detailed / precise in your comment.

after applying your second suggested change(which I assume makes the loop go on until all conditions have been evaluated or ...

My suggestion is for the while diff to be reduced to my previous comment. There shouldn't be any need for a loop in the first place.

[nepython]

@dirk-thomas, thanks for the quick response

What do you mean with this? Why is it being useful? Please try to be more detailed / precise in your comment.

I think I have mentioned it in the code snippet https://github.com/ros-infrastructure/catkin_pkg/pull/279#issuecomment-596962808 next to every line how it is helping in the short circuiting by stopping evaluation of further conditions in cases when we know that at the end result would be True(for or)/False(for and), I am mentioning the same below too.

There shouldn't be any need for a loop in the first place.

Is your suggestion to implement something like:-

if len(parse_results>3):
  if _evaluate(parse_results[0:3],context): #first condition evaluates to True
    if parse_results[3]=='or': #If it's a 'or' and condition evaluates to True, further evaluations can be skipped
      return True 
    #Recursion
    else:
      parse_results = parse_results[2:]
      _evaluate(parse_results,context)
  elif parse_results[3]=='or':  #If it's a 'or' and condition evaluates to False, still futher evaluation needs to be done
    parse_results = parse_results[2:]
    _evaluate(parse_results,context)
  else: #It's an 'and' and entire condition evaluates to False in that case
    return False
#Normal single condition evaluation 

[dirk-thomas]

Is your suggestion to implement something like:

No, my suggestion is literally in the above comment: https://github.com/ros-infrastructure/catkin_pkg/pull/279#issuecomment-596875191 The code snippets in that comment are the complete diff I think is needed.

[nepython]

parse_results[2] if len(parse_results) == 3 else parse_results[2:]

@dirk-thomas, I do get what you mean but can you please consider edge cases such as one below:- Suppose the condition is foo==bar and bar==bar or foo==foo (sayfoo!=bar) will lead to False, whereas in python it evaluates to True, hence I had put those extra checks to ensure that is also the case here.

Also, short-circuiting will help reduce the total recursion calls as it would simply return a value as I have explained above and not evalute all the conditions and then take decision, should definitely help.

Rest is your call, I can definitely make those changes just thought that few cases as the one I have mentioned above were amiss.

[dirk-thomas]

please consider edge cases such as one below

Then please add a unit test for that edge case ensuring that it does return the expected result.

[nepython]

Sure, does everything else in https://github.com/ros-infrastructure/catkin_pkg/pull/279#issuecomment-597167450 seems fine? I will squash all my commits and send one with these lines them finally.

[dirk-thomas]

does everything else in #279 (comment) seems fine?

No, as mentioned multiple time I don't see a reason to duplicate all this logic. If you really want to limit recursion then please do so in a separate PR. This one is to support multiple conditions.

[dirk-thomas]

@nepython Friendly ping.

[nepython]

@dirk-thomas, extremely sorry got a little busy. I have sent the patch as per your suggestions in comment. Please have a look. Shall I open another PR for limiting the number of recursions, this will help if there happen to be many conditions involved. :)

[dirk-thomas]

Shall I open another PR for limiting the number of recursions, this will help if there happen to be many conditions involved.

The recursion limit is 3000 (at least in Python 3.8) which should be plenty and I don't expect we ever have the case of that many conditions. But I will leave that up to you to decide - if you think it is valuable please open a new PR once this one has been merged.

[nepython]

@dirk-thomas, I think this is finally done on my side. Not sure why is Travis not running at all, might be down for maintenance. I have locally run nosetests -s --tests test and everything is passing.

The recursion limit is 3000 (at least in Python 3.8) which should be plenty and I don't expect we ever have the case of that many conditions.

I think then there is no need for it then. If you think there are still improvements please suggest I would be happy to make the necessary changes. :smile:

[dirk-thomas]

Not sure why is Travis not running at all, might be down for maintenance.

You might want to check https://www.traviscistatus.com/ for that information rather than just guessing.

Anyway I committed to tiny changes (see ae9fff5d2dfdc771416054b395250faf3fe618f6 and 657e0ec790b9faa53dfa50c9540ced24b85a603d) and Travis is processing the latest commit now.

[dirk-thomas]

Thanks for the improvement and for iterating on the ticket.