fixed MSCOLAB_timeout definition by list_operation_structure#2815
fixed MSCOLAB_timeout definition by list_operation_structure#2815ReimarBauer merged 4 commits intostablefrom
Conversation
ReimarBauer
changed the title
| text = f.read().decode("utf-8") | ||
| expected_version = __version__ | ||
| pattern = rf'value="is:closed milestone:{re.escape(expected_version)}"' | ||
| pattern = rf'value="is:closed milestone:{re.escape(expected_version)} "' |
There was a problem hiding this comment.
seems gh has changed the content.
The test is then very fragile and needs a better solution
mslib/utils/config.py
Outdated
| "gravatar_ids": ["[email protected]"], | ||
| "WMS_preload": ["https://wms-preload-url.com"], | ||
| "MSCOLAB_timeout": [[2, 10]], | ||
| "MSCOLAB_timeout": [int(), int()], |
There was a problem hiding this comment.
int() == 0, what's the reasoning for changing it to that? The main difference seems to be removing the inner list, but is that correct? Afterall, the timeout setting can be a tuple of two values, or something equivalent.
There was a problem hiding this comment.
Although it can be zero. These entries are in place to guarantee that new values have the same data type as existing ones. I’m hesitant to keep both default values because we used only placeholders elsewhere.
The inner list definition is wrong, it should only have 2 values of type integer.
There was a problem hiding this comment.
The inner list definition is wrong, it should only have 2 values of type integer.
Are you sure about that? The other definitions all have one element in the outer list, which defines a placeholder value for the config option. And in the case of MSCOLAB_timeout that config option is supposed to be a sequence of two elements, right?
There was a problem hiding this comment.
Yes, it is the same as with gravatar_ids, new_flightrack_template and some
more.
The list_option_structure is used by the config_editor, to create a new row for e.g. automated_plotting_flights [[]] with 6 elements.
using [] creates for the gravatar_id a new row with 1 element.
Because MSCOLAB_timeout is implemented as a list allowing additions and removals, this introduces potential problems in the UI. We need to ensure the UI behaves consistently with how it handles non-list values – for example
|
The editor does it now correct |
|
To maintain a unified approach, the logic needs to be consistently applied across the modify_config_file module, its tests, the configuration settings, and the user interface. That can be done with #2819 I am looking currently in that issue and write more tests :) |
ReimarBauer
changed the title
joernu76
left a comment
joernu76
left a comment
There was a problem hiding this comment.
The whole configuration scheme has gotten pretty complex to enforce the proper form of parameters... This should be simpler.
3 participants
Purpose of PR?:
Fixes #2716 #2819
Does this PR introduce a breaking change?
no
If the changes in this PR are manually verified, list down the scenarios covered::
xfailed test enabled
Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs
Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes
Checklist:
<type>: <subject>