这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@not-my-profile
Copy link
Contributor

What does this PR do?

Introduces about.language for non-English engines and sorts the tables after (disabled, about.language, name).

Before:

image

After:

image

Why is this change important?

It helps non-English speakers find engines of their language.

Related issues

#623

Copy link
Member

@return42 return42 left a 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])
Copy link
Member

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: (...)

Copy link
Contributor Author

@not-my-profile not-my-profile Dec 21, 2021

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.

Copy link
Member

@return42 return42 Dec 21, 2021

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.

Copy link
Contributor Author

@not-my-profile not-my-profile Dec 21, 2021

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.)

Copy link
Member

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 ..

@return42 return42 merged commit 06435e0 into searxng:master Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants