Skip to content

fix: Swallow redefinition warnings#72

Merged
lewisjared merged 3 commits into
mainfrom
supress-redefining-warnings
Dec 16, 2025
Merged

fix: Swallow redefinition warnings#72
lewisjared merged 3 commits into
mainfrom
supress-redefining-warnings

Conversation

@lewisjared
Copy link
Copy Markdown
Contributor

@lewisjared lewisjared commented Dec 16, 2025

Description

Swallows the redefinition warnings which are emitted on each import if logging is enabled.

Checklist

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Changelog item added to changelog/

@lewisjared lewisjared requested a review from znicholls December 16, 2025 06:42
@lewisjared
Copy link
Copy Markdown
Contributor Author

@znicholls I don't know how many million lines we have generated because of this. Any other suggestions about how we could handle this?

Copy link
Copy Markdown
Contributor

@znicholls znicholls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My small preference would be to add an initialisation argument to add_standards so we can control whether the warning is emitted or not (or perhaps better, just do the _on_redefinition trick around where add_standards is called rather than in add_standards).

Other solutions would require rewriting pint I suspect, so not really in scope (and pint's _on_redefinition attribute is probably good enough).

@lewisjared
Copy link
Copy Markdown
Contributor Author

@znicholls I added an argument to add_standards if you want to override with ignore or warn, defaulting to ignore.

@lewisjared lewisjared merged commit c183647 into main Dec 16, 2025
12 checks passed
@lewisjared lewisjared deleted the supress-redefining-warnings branch December 16, 2025 23:32

This overrides the default units registry behaviour for the duration of
this method. The previous behaviour is restored afterwards.
"raise" is not supported here as the redefinitions are required.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lewisjared I'm not sure this is correct. I wonder if it's possible to initialise a pint registry without inheriting the standard definitions, which would then mean that no redefinition is needed (maybe that's the mistake in openscm-units' construction, it inherits from the standard unit registry and therefore always leads to this redefinition).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand what isn't correct? The docstring is referring to the behaviour of the on_redefinition attribute of the unit registry.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the redefinitions are required" is the bit that I'm not sure is correct. The way we have implemented things, the redefinitions are required, but I'm not sure that's true in the absolute sense (i.e. a different way of implementing may have meant this redefinition wasn't required).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see. We should move that to an issue as that is a breaking change and was a much larger undertaking than make the logs go away 😁

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#74

@lewisjared lewisjared mentioned this pull request Dec 18, 2025
@znichollscr
Copy link
Copy Markdown
Contributor

znichollscr commented Dec 18, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants