fix: warn on deprecated images being set #2312
Conversation
Signed-off-by: Henry Schreiner <[email protected]>
henryiii
force-pushed
the
henryiii/fix/warn-on-dep
branch
from
a4ed9aa to
9fa0075
Compare
Signed-off-by: Henry Schreiner <[email protected]>
cibuildwheel/logger.py
Outdated
| @@ -164,7 +165,11 @@ def notice(self, message: str) -> None: | |||
| c = self.colors | |||
| print(f"cibuildwheel: {c.bold}note{c.end}: {message}\n", file=sys.stderr) | |||
|
|
|||
| def warning(self, message: str) -> None: | |||
| def warning(self, message: str, *, deduplicate: bool = False) -> None: | |||
There was a problem hiding this comment.
Maybe we could just always do this for warnings?
There was a problem hiding this comment.
Think I'd prefer to keep it explicit, it's a bit surprising at the call site otherwise. Might also make some unit tests buggy as this is shared global state.
There was a problem hiding this comment.
^ actually the global state thing might be a reason to do it on Options rather than here.
There was a problem hiding this comment.
I was originally going to make it global as a ClassVar in options. I guess if it was an instance variable it still wouldn't over trigger? It's pretty common for warning to be only shown once globally (the "default" setting for warnings in Python, for example).
There was a problem hiding this comment.
I guess if it was an instance variable it still wouldn't over trigger?
I don't think so. There's only one instance of Options in a normal running process. The only reason I hesitate to do it globally is that I can imagine a unit test expecting to see a warning but depending on test execution order, it sometimes gets triggered somewhere else and the test becomes flaky/hard to debug.
That said it's a hypothetical issue at this point, we can leave it here if you wish.
cibuildwheel/logger.py
Outdated
| @@ -164,7 +165,11 @@ def notice(self, message: str) -> None: | |||
| c = self.colors | |||
| print(f"cibuildwheel: {c.bold}note{c.end}: {message}\n", file=sys.stderr) | |||
|
|
|||
| def warning(self, message: str) -> None: | |||
| def warning(self, message: str, *, deduplicate: bool = False) -> None: | |||
There was a problem hiding this comment.
Think I'd prefer to keep it explicit, it's a bit surprising at the call site otherwise. Might also make some unit tests buggy as this is shared global state.
Co-authored-by: Joe Rickerby <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
henryiii
force-pushed
the
henryiii/fix/warn-on-dep
branch
from
1c3d5c4 to
8a190f4
Compare
* fix: warn on deprecated images being set Signed-off-by: Henry Schreiner <[email protected]> * fix: handle repeated warning messages Signed-off-by: Henry Schreiner <[email protected]> * Apply suggestions from code review Co-authored-by: Joe Rickerby <[email protected]> * fix: try making the warnings once per Options Signed-off-by: Henry Schreiner <[email protected]> --------- Signed-off-by: Henry Schreiner <[email protected]> Co-authored-by: Joe Rickerby <[email protected]>
See #2047.