-
-
Notifications
You must be signed in to change notification settings - Fork 714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat-3416: configurable-output-name-tags #3486
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the formatting conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:XX
add "name" tolang_tags
- for
name
add "name" totags
, check iflang_tags
has elements then call_add_lang_tags(*lang_tags)
and initializelang_tags = []
again.
In this way tags are processed in batches
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_NAMES
as suggested in the issue #3416.Description
_build_default_output_name_tags
_build_output_name_tags
NOMINATIM_OUTPUT_NAMES
is providedname:XX,name,brand,official_name:XX,short_name:XX,official_name,short_name,ref
:XX
identifies lang tags:XX
identifies multiple lang tags to-be added at the same timeRelated Issue
#3416
Motivation and Context
NOMINATIM_OUTPUT_NAMES
as suggested in the issue.How Has This Been Tested?
test_default_output_name_tags
tests the default setting (previously hardcoded) in case environment config is not providedtest_configurable_output_name_tags
tests 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