feat-3416: configurable-output-name-tags #3486
Conversation
|
The issue didn't ask for an environment variable but for a configuration setting. |
|
I see that configuration setting is managed by dotenv as quoted in Configuring Nominatim
This means that Please correct me if there is mistake. |
|
https://nominatim.org/release-docs/latest/library/Configuration/ has some more details on the Configuration object, which holds the configuration options that have been loaded. You shouldn't query the configuration in the Locale class directly though. Just hand in an optional output like in #3451 and make sure the current configuration is handed in at the appropriate places. |
|
Now accessing the setting via configuration object which can be provided optionally. Please review. |
|
@lonvia please suggest if there are further changes required. And let's discuss some more issues to which I can contribute. I am eager to dive deeper in to the core programming of python. |
There was a problem hiding this comment.
Your code so far has no effect when actually running Nominatim because you haven't changed anything in the code where Locale() is used. Please set up an installation of Nominatim, and test your changes on it. You should be able to observe the effects of changes to your new configuration variable with both, nominatim serve and nominatim search.
When adding a new configuration option, you also need to add a default in https://github.com/osm-search/Nominatim/blob/master/settings/env.defaults and documentation in https://github.com/osm-search/Nominatim/blob/master/docs/customize/Settings.md.
src/nominatim_api/localization.py
Outdated
| """ | ||
|
|
||
| def __init__(self, langs: Optional[List[str]] = None): | ||
| def __init__(self, langs: Optional[List[str]] = None, config: Optional[Configuration] = None): |
There was a problem hiding this comment.
I'd like to avoid a dependency on Configuration. Please just hand in the content on the configuration option, i.e. a string.
There was a problem hiding this comment.
I am puzzled on how to use the config directly to access the configuration, hence modified the args and accepting the config value directly
src/nominatim_api/localization.py
Outdated
| - Coma separated | ||
| - `:XX` identifies lang tags | ||
| - consecutive `:XX` identifies multiple lang tags to-be added at the same time | ||
| - subsequent parts after lang tags (till next lang tag) identifies batch of tags |
There was a problem hiding this comment.
Please keep the formatting conventions.
There was a problem hiding this comment.
I am not sure about the mistake here, So I rewrote the class doc string in a format I found on official website
src/nominatim_api/localization.py
Outdated
| """ | ||
| nominatim_output_names = nominatim_output_names.split(",") | ||
| lang_tags = [] | ||
| tags = [] |
There was a problem hiding this comment.
This seems wrong. Order matters here. The name tag list must be in exactly the same order as the input list.
There was a problem hiding this comment.
I need more context on this. These variables are initialized to build the tags in order as specified in the format, they work in batches.
consider default: name:XX,name,brand,official_name:XX,short_name:XX,official_name,short_name,ref
- for
name:XXadd "name" tolang_tags - for
nameadd "name" totags, check iflang_tagshas elements then call_add_lang_tags(*lang_tags)and initializelang_tags = []again.
In this way tags are processed in batches
|
My main comment: "Your code so far has no effect when actually running Nominatim because you haven't changed anything in the code where Locale() is used. " still hasn't been addressed. This is not getting us anywhere. Closing. |
This PR makes the hard-coded list of output name tags configurable, addressing issue #3416. It allows users to specify their preferred output name tags via an environment setting
NOMINATIM_OUTPUT_NAMESas suggested in the issue #3416.Description
_build_default_output_name_tags_build_output_name_tagsNOMINATIM_OUTPUT_NAMESis providedname:XX,name,brand,official_name:XX,short_name:XX,official_name,short_name,ref:XXidentifies lang tags:XXidentifies multiple lang tags to-be added at the same timeRelated Issue
#3416
Motivation and Context
NOMINATIM_OUTPUT_NAMESas suggested in the issue.How Has This Been Tested?
test_default_output_name_tagstests the default setting (previously hardcoded) in case environment config is not providedtest_configurable_output_name_tagstests the output name tags when environment config is providedChange will not affect other areas of code since previous behavior is preserved while enhancing the configurability if required