TST: fix a logic flaw in a pytest fixture#19567
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
| """ | ||
| old = sys.getswitchinterval() | ||
| sys.setswitchinterval(1e-6) | ||
| sys.setswitchinterval(min(old, 1e-6)) |
There was a problem hiding this comment.
If old is indeed smaller than 1e-6, shouldn't this be a no-op instead?
There was a problem hiding this comment.
That is exactly what I meant to write. Is there an inconsistency I'm not seeing ?
There was a problem hiding this comment.
I mean if min(old, 1e-6) is still old, why call sys.setswitchinterval(...) at all?
There was a problem hiding this comment.
Just a style preference.
There was a problem hiding this comment.
The (admittedly small) gain is that I don't need to refer to the variable storing the new interval more than once, so there's 0 chance of two occurrences drifting apart.
There was a problem hiding this comment.
What about this? 😅
old = sys.getswitchinterval()
new = 1e-6
if old > new:
sys.setswitchinterval(new)
yield
if old > new:
sys.setswitchinterval(old)There was a problem hiding this comment.
4 references to old , as many to new, and 5 LOCs where one 2 suffice is just worse in my honest opinon.
There was a problem hiding this comment.
Well, the stated goal is to fix flaw, not to minimize LOC 😆
@astrofrog do you have second opinion? 🙏
|
in any case, this is just fixing a hypothetical defect that to my knowledge has no actual impact in existing systems, so I'm moving it to 8.1.0 to ever so slightly reduce 8.0.0's backlog |
Description
Technically this is a fix to a logic flaw (we should not update the switching frequency unconditionally), but not worth backporting; it's merely theoritical.