Skip to content
This repository has been archived by the owner on Sep 7, 2023. It is now read-only.

Created new plugin type custom_results. Added new plugin bang_redirect #2027

Merged
merged 57 commits into from Jul 3, 2020

Conversation

lukasvdberk
Copy link
Contributor

TLDR

  • Created a new plugin type of custom_results
    • Documented the new plugin type and usage in plugins.rst
  • Created a new plugin called "Bangs redirect". This will redirect the user directly to the site (with the query) instead of showing the results on the results page. Also included a extra config for this plugin (containing 7.5k sites)
  • Created unit tests for my "Bang redirect" plugin.

Detailed version
I really like Searx and have only been using it for 2 weeks. Coming from DuckDuckGo I really liked their implementation of bangs and wanted to recreate that for Searx. Their implementation is when you enter a bang it redirects directly to the site with your submitted query. Maybe I have missed something but I could not find that for Searx. I also wanted more bangs out of the box. Thats why I used by own bangs config data (located in searx > plugins > bangs_data > bangs.json).

For all of this to work I created a new plugin type called custom_results (which is documented and anyone can use ). With this you can return a custom flask return type so you can for example redirect (like I used) or show a custom page. It runs when a user submitted a query (just like post_search) and will cancel the rest of the usual search flow when it does not return None.

I also added unit_tests in tests > unit > test_plugins > SelfBangTest. This will test my Bang redirect plugin for all of its features. The tests ran successfully on my machine with python 3.7

I retrieved my bangs data from the following project , the data file (I converted the TOML file to a JSON file).

If you want to see all of this working you can checkout my instance at https://searx.lukashisprojects.rocks/. Make sure to enable the plugin in the preferences section.

Personal note
I am still a cs-student and this is my first opensource contribution. I really love this project and I hope I have made a useful contribution. If there is anything I can do to make this PR work I am willing to go the extra mile.

@abcdan
Copy link

abcdan commented Jun 26, 2020

Interesting, really like the idea.

@return42
Copy link
Contributor

Thanks! .. before we add new plugins #1938 should be merged. The call_with_results should be implemented in searx. BTW I want to link #1146

@lukasvdberk
Copy link
Contributor Author

@return42 Thanks for the quick response! I guess I will just wait before making other changes when #1938 is merged. If I can anything else to better fit this plugin mention me.

Copy link
Member

@asciimoo asciimoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it's a good PR. I've added some minor notes, please check it.

return full_query


# Dont use this variable directly but access via _get_bangs_data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't use the variable directly? In the end this variable is being used anyway since _get_bang_data() returns this variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer getters and setter but with your example below it is alternative way (your parser is way nicer/cleaner tho!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it!

bangs_data = None


def _get_bangs_data():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit more compact and pythonish definition of this section:

bangs_data = {}
with open(join(searx_dir, 'data/bangs.json')) as json_file:
   for bang in json.load(json_file)['bang']:
      for trigger in bang["triggers"]:
         bangs_data[trigger] = {x:y for x,y in bang.items() if x != "triggers"}

No need to create a loader function, this way bangs_data is populated immediately after starting the application, then we can use it freely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

     bangs_data[trigger] = {x:y for x,y in bang.items() if x != "triggers"}

Wow I did not know you could do that in 1 line btw. Learned something new thanks!

:param user_bang: The parsed user bang. For example yt
:return: Returns a dict with bangs data (check bangs_data.json for the structure)
"""
try:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can simply write return _get_bangs_data().get(user_bang). dict().get(key) returns None if the given key doesn't found.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it!

@lukasvdberk
Copy link
Contributor Author

@dalf @asciimoo

The feedback you gave was really valuable and I have implemented it all (or commented).
Is it a good idea that you resolve the conversations (if it is off course all correct)? (or comment back).

Really thanks 👍

@asciimoo
Copy link
Member

Tests are failing because the result_container in the webapp unit tests is mocked: https://github.com/asciimoo/searx/blob/master/tests/unit/test_webapp.py#L50 . You have to add the new redirect_url=None attribute to the mock object which will solve the currently failing tests, but the new test you added will fail because redirect_url is None in the mock object.
Probably https://github.com/asciimoo/searx/blob/master/tests/unit/test_search.py would be the proper place to test this new feature.
Could you fix the webapp tests by extending the result_container mock object and could you move the new test to test_search.py?
Lastly, a commit fixup would be appreciated as a final touch to keep the git log clean and I think after that it's ready to merge.

@lukasvdberk
Copy link
Contributor Author

lukasvdberk commented Jul 1, 2020

Tests are failing because the result_container in the webapp unit tests is mocked: https://github.com/asciimoo/searx/blob/master/tests/unit/test_webapp.py#L50 .

Oh got it! Added the mock all tests are passing now. Thanks for the tip!

Probably https://github.com/asciimoo/searx/blob/master/tests/unit/test_search.py would be the proper place to test this new feature.

I agree and I refactored it.

Lastly, a commit fixup would be appreciated as a final touch to keep the git log clean and I think after that it's ready to merge.

I did not know this existed but I tried to implement it(don't know If I did it correctly). The you link sended for fixup was helpful for the explanation but I did not got it working. I followed this video maybe is good to add to reference in future PR's maybe? @asciimoo

@abcdan
Copy link

abcdan commented Jul 1, 2020

Looks pretty good to me!

searx/external_bang.py Outdated Show resolved Hide resolved
Copy link
Member

@asciimoo asciimoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@asciimoo asciimoo merged commit 4829a76 into searx:master Jul 3, 2020
@lukasvdberk
Copy link
Contributor Author

lukasvdberk commented Jul 3, 2020

Looks good to me, thanks!

Im really grateful to have worked with you guys @dalf and @asciimoo ! You provided good feedback and gave really good insights of how the code should be structured to made this PR work (learned some stuff to apply to my own projects).

One last thing. Should I add documentation and if so what should I include?
I am thinking of the usage of the external bang where to find the external bangs and maybe how to add your own bang.

@return42
Copy link
Contributor

return42 commented Jul 3, 2020

I can write the documentation .. I want to auto generate the list of bangs in the docs, this might be to complicate if this is the first time you are using the sphinx framework (reST).

@lukasvdberk
Copy link
Contributor Author

lukasvdberk commented Jul 3, 2020

@return42 If you want to do that would be awesome, since my English and writting skills are not so great..... But if you have any questions I am always available to help!

@return42
Copy link
Contributor

return42 commented Jul 5, 2020

@return42 If you want to do that would be awesome, since my English and writting skills are not so great.

@lukasvdberk believe me, it is better than mine ;)

To document the bangs I send PR #2046. You might have been noticed that I also opened issue #2045 in which I call for a significant reduction of bangs (redirect URLs) .. your thoughts and remarks are welcome / thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants