Reuse Configuration instance in Locales #3913
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AyushDharDubey
changed the title
lonvia
reviewed
Dec 17, 2025
Member
lonvia
left a comment
lonvia
left a comment
There was a problem hiding this comment.
The constructor remains forgiving by allowing config=None for call sites where a configuration object is not available in context for example.
Looking at the line you cite here:
Nominatim/src/nominatim_api/v1/format.py
Line 135 in 5e965d5
I notice it was badly written. Locale() will always be instantiated here, even when it is available in the options dict. This should be: locales = options.get('locales') or Locales().
And I don't think the code path where Locales() is instantiated is actually reachable afterwards.
lonvia
reviewed
Dec 18, 2025
lonvia
reviewed
Dec 19, 2025
Member
lonvia
left a comment
lonvia
left a comment
There was a problem hiding this comment.
Do you mind squashing together your commits into a single one. Then this would be ready to merge.
expect output names as argument and avoid redundant configuration initialization
AyushDharDubey
force-pushed
the
fix/issue_3909
branch
from
a99953d to
4fd6162
Compare
Member
|
Thank you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #3909
This PR rewires the
Localesconstructor to accept an existingConfigurationinstance via aconfig=parameter, instead of instantiating a new (empty) configuration on each call.The constructor remains forgiving by allowing
config=Nonefor call sites where a configuration object is not available in context for example.By reusing a single
Configurationinstance where possible, this avoids repeated configuration loading and significantly reduces per-request overhead by over 25-30% per request.