Avoid mutating _filter dict for subsequent calls

[mbladel]

Fixes the code introduced with #217 that flattens the labels list to a comma-separated string by avoiding mutating the original _filter object.

[ralphbean]

/ok-to-test

[webbnh]

Looks good. Are you planning on providing a similar change to test_upstream_pr.py?

[mbladel]

@webbnh thanks for the review! The test for Para would basically be identical, so I don't think that's necessary.

[webbnh]

The test for Para would basically be identical, so I don't think that's necessary.

Yes it would be basically identical, but what it would be testing would be completely different! 🤣

Yes, we don't like duplicated code, but that applies more to the product than to the tests. Having duplicated code there is evil, too, but having duplication is better than suffering omission.

If we were being really proper, we would refactor both the issue code and the PR code and move the duplication to a common utility routine, and then we would need only a single instance of the test, because there would be only a single instance of the code. (And, in fact, I have a branch full of such refactorings, but we haven't summoned the collective will to get it merged.)

[webbnh]

My suggestion to keep the mutation of the in-memory config object is probably not a good idea without additional changes. 😢

We have code elsewhere (which I think you are familiar with 😉) which expects the labels to be a list and not a comma-separated string! (And, Python doesn't issue an error if the argument to isdisjoint() is a string, because str's are iterable, and it turns the string into a set of its individual characters, and then returns True if all of the labels are multi-character strings.)

So, it would probably be best to maintain the list as a list and only flatten it locally. Sorry for the misdirection! (Clearly, there is an "opportunity" here to improve our testing!)

[mbladel]

Right @webbnh, so I reverted to my previous solution - hopefully that finally does it!

[mbladel]

@webbnh feel free to push the best version of the code to my PR or even close it and open one yourself - I'm far from a Python expert but the dictionary comprehension looked more concise and worked, I'm sorry I don't really have the time to dedicate to perfecting this one little change our team needed (and I though I could contribute quickly). I appreciate the tips, though timezone difference doesn't make this easy.

Did one additional push, not sure about the test for PRs (it's exactly the same copy-pasted code), but here you go :)

[webbnh]

LGTM

@mbladel, thanks for your patience and for making the effort! None of us are Python experts (and, I'm not convinced that Python experts are even useful, because "expert Python" tends to be hard to read and even harder to understand).

Everything that we do is a series of trade-offs -- the choices of code idiom, the amount of testing, how to spend our engineering time -- and ultimately, each of us has to make his/her own choice as to how those trades should go. In terms of my code review, I'm only trying to make you aware of what those choices are (and, admittedly, to influence you in some cases 😉), but ultimately you need to make the choice yourself. (And, if I'm unhappy with your choice, as you say, I can choose to post my own PR! 😁)

And, yeah, the timezone difference poses problems in various ways.

the dictionary comprehension looked more concise

The problem is that conciseness isn't the only (or best) criterion. We want the code to be easily understood by a reader while still being reasonably efficient. To my eye, multi-line comprehensions push the boundary, and the embedded ternary compounds the problem. (Don't get me wrong, I love ternaries, but I kind of hate Python's syntax for them.) So, for that particular case, I advocate for avoiding it (particularly, since you're basically copying the the dict and changing one key's value, it seems like using deepcopy() better expresses your intention, and it might be more efficient).

not sure about the test for PRs

The code seems reasonable to me, and, more importantly, to Tox. 😀 Thanks!

[mbladel]

@ralphbean ok to merge this? Thanks!

[ralphbean]

/ok-to-test

[ralphbean]

/ok-to-test

[ralphbean]

/ok-to-test

[mbladel]

@ralphbean not sure why we're not merging this, is there a blocker?

[ralphbean]

/ok-to-test

[ralphbean]

/retest

[ralphbean]

/ok-to-test