-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[mod] package.html template: additional links (a python dict) #3464
Conversation
Please also update the documentation at https://github.com/searxng/searxng/blob/master/docs/dev/engines/engine_overview.rst and add the new field there too! |
367fe77
to
8a400c8
Compare
I updated the pr, it's ready for re-review |
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 personally think it would be better to keep source_code_url
, documentation_url
and homepage
normal fields (not part of the dict), because they do behave differently than normal links
. It's a good idea to display them together, but for the developer experience, they should stay separated because one would otherwise always need to look up what specific edge cases for the links
dictionary exist (as it's not properly documented, and doesn't follow the way other templates work).
The GitHub engine would require modifications too (source_code_url
and homepage
) looking at the current changes. But when following the suggestion above, it won't need to be modified.
After thinking about it, I think the easiest way would be to link to the documentation as homepage. On the other hand hex does not have a standardized list of keys in the link dictionary and in the example how to publish a package there is only github so this is the only one people add in 95%. But it's not typed and can be written in different casing - some examples I printed out, sometimes there may be links to other packages on github too - when I'm wrapping a rust library so it can be used in elixir I'm adding links to it. {'Authority (GitHub)': 'https://github.com/infinitered/authority', 'Authority (Hex)': 'https://hex.pm/authority', 'GitHub': 'https://github.com/infinitered/authority_ecto'}
{'Changelog': 'https://github.com/TheFirstAvenger/ecsx_persistence_ecto/blob/master/CHANGELOG.md', 'GitHub': 'https://github.com/TheFirstAvenger/ecsx_persistence_ecto'}
{'Docs': 'https://hexdocs.pm/aws_ex_ray_ecto', 'Github': 'https://github.com/lyokato/aws_ex_ray_ecto'}
{'Docs': 'https://hexdocs.pm/couchx', 'GitHub': 'https://github.com/javierg/couchx'}
{'Documentation': 'https://hexdocs.pm/carguero_ex_audit', 'GitHub': 'https://github.com/carguero/carguero_ex_audit'}
{'GitHub': 'https://github.com/elixir-ecto/ecto'}
{'GitLab': 'https://github.com/tlux/database_yaml_config_provider'}
{'Github': 'https://github.com/sheharyarn/ecto_atomized_map'}
{'Gitlab': 'https://gitlab.com/patatoid/ecto3_mnesia'}
{'Homepage': 'https://buildkite.com/test-analytics', 'Source': 'https://github.com/buildkite/test_collector_elixir'}
{'Homepage': 'https://git.centralstandard.com/open-source/date-params'}
{'github': 'https://github.com/MorphicPro/dissolver'}
{'github': 'https://github.com/rstacruz/authsense'}
{} |
I think that the I just didn't like the way you handled the translation of the strings for So my take would be - take the commit 8a400c8 as base and just make |
The translations could be moved to the engine itself instead of template - then the for loop can take 5 lines. I started to generate the links in a loop because currently it's using multiple if statements to insert the Keeping the existing url's and adding separate logic with for loop would still be a bit ugly and take around 20 lines. |
I think that source code and homepage are available for almost all package engines at SearXNG, so the translation should not be done by each engine individually to reduce boilerplate code. |
Looks good to me now from the idea, I'm just thinking about whether it would make sense to move the |
In hex there is only a list of licenses available so I can't do it in this particular example - It's like tags. |
Yes, I see, I was rather thinking about the template for packages in general (GitHub, NPM, ...) |
@dkuku thanks a lot for your work on this topic 👍 .. I'm going to review this PR and #3487 .. both work on packages.html, so I will start with this PR .. right? .. #3487 is approved by @Bnyro so I assume this modification of packages.html is accepted by both of you in sense of #3456 .. which can be closed when this PR (#3464) is merged / right? In the course of the changes to the PR here, the target, the package.html was taken out in the meantime and the patch history as well as the title of this PR are unclear regarding this target ... @dkuku is it okay for you if I clean up the commit history and also correct the title of this PR again? (if I have interpreted the course of the PR correctly with my assumptions above). |
@return42 👍🏻 edit both pr-s it too your liking and merge it when done. |
bb1d3e3
to
7dc6a36
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.
@dkuku @Bnyro I squashed the commit history into one commit and slightly changed the commit message. And I did some small modifications / see diff
See my comments in this review below for more details .. if you are OK with what I have done, give a short feedback to merge this PR.
Again, thanks a lot for your work on this PR 👍
Looks good to me. |
The names of the links are rather tags than real names, and they sometimes vary greatly in their spelling: - GitHub: github, Github - Source code: Repository, SCM, Project Source Code - Documentation: docs, Documentation It was standardized to terms such as 'Source code' and 'Documentation', as translations already exist for these terms. Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>
What does this PR do?
Refactor package template to include custom amount of links
Rename publishedDate variable to published_date
change engines that use this variable
Why is this change important?
How to test this PR locally?
Author's checklist
Related issues