1

Starting with the following list comprehension: 

new_list = [re.sub(r'replace_a', 'with__b', i)
             if not re.findall(r'find_pattern', i, re.M) else 
            re.sub(r'replace_c', r'with__d', i)
            for i in old_list]

Now I would like to add an if condition after else that replaces another pattern using re.sub(). I tried with:

new_list = [re.sub(r'replace_a', 'with_b', i)
             if not re.findall(r'find_pattern', i, re.M) else
           (re.sub(r'replace_c', r'with_d', i)
             if foo == re.match("pattern", foo))
           for i in old_list]

Tried to figure out the proper way to do it with list comprehensions but had no luck.

faber
  • 57
  • 6
  • 1
    The final `if` clause needs to go at the end I think since it's dosen't have an assosed `else`. You could write out a `for` loop you know, or at least add some line breaks to make this readable – Chris_Rands Mar 29 '18 at 14:27
  • What's the error you are getting? I think you might just need to add in a final `else`, because it looks like this pattern other than that https://stackoverflow.com/a/9987533/1011724. Or maybe try remove the parenthesis around you last `else`...`if` clause. But also `foo == re.match("pattern", foo)` doesn't depend on anything in the list comprehension so are you trying to make that entire clause conditional on something external? Edit your Q to add line breaks, explain your desired logic and drop the regex and add an [MCVE](https://stackoverflow.com/help/mcve). – Dan Mar 29 '18 at 14:27
  • The problem isn't the list comprehension; it's the conditional expression that is missing its `else`. – chepner Mar 29 '18 at 14:32
  • @Chris_Rands apologies, added the line breaks. I tried with the for loop but I have problems when I append the elements to the new list. – faber Mar 29 '18 at 14:42
  • @Dan yes, as you said it missed the final else. With `foo = re.match` I wanted to add an additional condition based on the value of a variable – faber Mar 29 '18 at 14:47

2 Answers2

5

This will read much more easily as a regular for loop:

new_list = []
for i in old_list:
    if not re.findall(r'find_pattern', i, re.M):
        new_list.append(re.sub(r'replace_a', 'with_b', i))
    elif foo == re.match("pattern", foo):
        new_list.append(re.sub(r'replace_c', r'with_d', i))
    # else:
    #    i

Your problem is that a conditional expression must always take an else clause, because it has to have some value whether or not the condition is true. With an if statement, however, you can omit the else. The above, for instance, has the ability to make new_list shorter than old_list, as not every i needs to result in a call to new_list.append. Uncommenting the else results in the same result as jpp's answer.

If you insist on using a list comprehension, then you can format it to make it more readable. Consider

new_list = [re.sub(r'replace_pattern_a', 'with_pattern_b', i)
               if not re.findall(r'find_pattern', i, re.M) else 
            re.sub(r'replace_pattern_c', r'with_pattern_d', i) 
               if foo == re.match("pattern", foo) else
            i
            for i in old_list]

although the conditional expression really isn't designed for such nesting. This visually separates the expressions that could be added to the new list from the conditions used to make the decision, but I'm not a big fan. There are other ways to format that make different trade-offs, but IMO the regular for loop is superior.

As jpp mentions, another way to refactor is to define a generator function:

def foo(old_list):
    for i in old_list:
        if not re.findall(r'find_pattern', i, re.M):
            yield re.sub(r'replace_a', 'with_b', i))
        elif foo == re.match("pattern", foo):
            yield re.sub(r'replace_c', r'with_d', i))
        else:
            yield i

new_list = list(foo())

which would have its pros and cons as well. (I think enumerating those is probably beyond the scope of this answer, but it does fall somewhere between the two extremes of a single list comprehension and an explicit for loop. It also comes closest to the construct I miss from my Perl days, the do statement, which is something like a lambda that doesn't take any arguments, but can contain arbitrary statements.)

chepner
  • 497,756
  • 71
  • 530
  • 681
  • thank you very much for your detailed explanation and for the solution using a `for` loop. I haven't thought of putting `re.sub` in new.list(append). Have a lot to learn. ~faber – faber Mar 29 '18 at 15:03
  • 1
    Looking at it more, I'd probably use a temporary variable anyway. `x = re.sub(...); new_list.append(x)`. That also has the benefit of letting you defer the call to `new_list.append` until after the `if` statement. Just set `x` as appropriate in each branch, then call `new_list.append(x)` at the end. – chepner Mar 29 '18 at 15:04
  • 1
    In my opinion, an even more readable option is to use a generator function and `yield` items. Then call `list` when required. Doubt there's any performance improvement here, but this removes boilerplate code of instantiating a list and using `list.append`. – jpp Mar 29 '18 at 15:06
  • chepner and jpp, thank you again. I will definitely take in your suggestions to better code. – faber Mar 29 '18 at 15:14
  • @chepner thank you for showing the solution with a generator function. Very interesting and plus, it looks neat. – faber Mar 29 '18 at 15:36
2

You need to specify an else condition for when none of the conditions are met. For example:

import re

old_list = ['a', 'b', 'c']

new_list = [re.sub(r'replace_pattern_a', 'with_pattern_b', i) if not re.findall(r'find_pattern', i, re.M) \
            else re.sub(r'replace_pattern_c', r'with_pattern_d', i) if foo == re.match("pattern", foo) \
            else i for i in old_list]

# ['a', 'b', 'c']

However, this is unreadable. I suggest, if performance is not an issue, you create a function to provide this manipulation with standard if / elif / else constructs.

jpp
  • 159,742
  • 34
  • 281
  • 339