-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[doc] introduce about.language and sort engines by it #629
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
Conversation
e821d0d to
f6aa09d
Compare
f6aa09d to
e28c6bd
Compare
return42
left a comment
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.
LGTM thanks a lot! .. I made some remarks, see below .. what do you think?
| 'sort_engines': | ||
| lambda engines: sorted( | ||
| engines, | ||
| key=lambda engine: (engine[1].disabled, engine[1].about.get('language', ''), engine[0]) |
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.
Nit: can you add a space left & right to the equation symbol / even in arguments / I know it is not PEP8, but it is much better to read ;)
key = lambda engine: (...)
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'm happy to adhere to any formatting style you choose for your project. I'm however also a fan of consistency and currently the code mostly sticks to the PEP8 style. Given that there's a recent PR to introduce auto-formatting #619, arbitrarily applying manual formatting here seems weird to me.
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'm happy to adhere to any formatting style you choose for your project. I'm however also a fan of consistency and currently the code mostly sticks to the PEP8 style.
Our code is (mostly) PEP8 / but not in this one case: What PEP8 says about formatting arguments is good for one-liners but not for a multiline argument list, especial not where the value of an argument takes several lines (IMO PEP8 is pointless in this case).
Given that there's a recent PR to introduce auto-formatting ..
Yes we have #619 but this PR is in a draft state and I don't know when we have finished a code-style workflow for python like we have done this month for JS.
jinja_filters = {
'sort_engines': lambda engines: sorted(
engines,
key = lambda engine: (
engine[1].disabled, engine[1].about.get('language', ''), engine[0]
),
)
}
.. is much better to read / nevertheless what PEP8 says .. AFAIK black won't fix this to PEP8 style.
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.
$ grep -Pzohr '[(,][\n\t ]*[a-z]+=[^\n]+' --include='*.py' searx | tr "$'\0'" $'\n' | grep = | wc -l
413
$ grep -Pzohr '[(,][\n\t ]*[a-z]+ =[^\n]+' --include='*.py' searx | tr "$'\0'" $'\n' | grep = | wc -l
77
The PEP8 style seems way more common in the codebase.
AFAIK black won't fix this to PEP8 style
Actually it does. (Black is in general very strict to prevent such discussions.)
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.
Actually it does.
Seems you are right .. I will never understand why this arbitrary setting of whitespaces ..
dict_a = {
"foo": "bar",
"john": "doe",
}
dict_b = dict(
foo="bar",
john="doe",
)
should be more readable than ..
dict_a = {
"foo" : "bar",
"john" : "doe",
}
dict_b = dict(
foo = "bar",
john = "doe",
)
.. anyway if the majority thinks every detail of PEP8 weights more than readability ..
What does this PR do?
Introduces
about.languagefor non-English engines and sorts the tables after (disabled, about.language, name).Before:
After:
Why is this change important?
It helps non-English speakers find engines of their language.
Related issues
#623