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] plugins: new unit converter plugin #3378
Conversation
16f076d
to
ec04365
Compare
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.
Note: 42 F in C
does not work
searx/sxng_locales.py
Outdated
@@ -56,6 +56,7 @@ | |||
('fr-CA', 'Français', 'Canada', 'French', '\U0001f1e8\U0001f1e6'), | |||
('fr-CH', 'Français', 'Suisse', 'French', '\U0001f1e8\U0001f1ed'), | |||
('fr-FR', 'Français', 'France', 'French', '\U0001f1eb\U0001f1f7'), | |||
('gl', 'Galego', '', 'Galician', '\U0001f310'), |
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.
can you remove this hunk from the patch / I assume its a leftover when you updated the searx/data/wikidata_units.json
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.
Oh yeah sorry, I didn't notice make data.all
also modifies sxng_locales.py
, I've only had reverted everything in searxng/data
.
I think this is due to the ambiguity of symbol However: "Q1396128": {
"si_name": "Q25406", -- Coulomb
"symbol": "F",
"to_si_factor": 96485.33212331001
}
"Q131255": {
"si_name": "Q131255", -- Farad
"symbol": "F",
"to_si_factor": 1.0
}
"Q25406": {
"si_name": "Q25406", -- Coulomb
"symbol": "C",
"to_si_factor": 1.0
},
"Q69362731": {
"si_name": "Q69363953", -- kelvin difference
"symbol": "°C",
"to_si_factor": 1.0
},
"Q42289": {
"si_name": null,
"symbol": "°F",
"to_si_factor": null
}, The issues are addressed by: |
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.
Thanks for unit conversion 👍 / I think this is a very good initial version .. I'm going to merge, may we can improve unit handling in a follow up PR ..
for unit in WIKIDATA_UNITS.values(): | ||
if unit['symbol'] == from_unit_key: | ||
from_unit = unit | ||
|
||
if unit['symbol'] == to_unit_key: | ||
to_unit = unit | ||
|
||
if from_unit and to_unit: | ||
break | ||
|
||
if from_unit is None or to_unit is None or to_unit.get('si_name') != from_unit.get('si_name'): | ||
return |
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.
IDK if possible but may we can improve unit detection where symbols are ambiguous or si_name
is null
.. see #3378 (comment)
Oh yes, we only filter for letters here (and no ° symbol): https://github.com/searxng/searxng/pull/3378/files#diff-1a8835e42547f530fd3a8267f9ba44b660670c9cd0b0a7b0c15bd77c53e9a43eR30 We can probably just allow any kind of symbols that are not numbers for the unit name. |
Seems we have an issue in the CI: |
This patch is a leftover from [1] in which the WIKIDATA_UNITS values has become a dictionary. [1] searxng#3378 Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
This patch is a leftover from [1] in which the WIKIDATA_UNITS values has become a dictionary. [1] #3378 Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
What does this PR do?
Why is this change important?
How to test this PR locally?
Author's checklist
Related issues
closes #3015
Would be great if you could have a look at these changes @dalf, there might be a better and safer way to implement
_parse_text_and_convert()
.