Fixes #472 Add News search support for Mojeek #473
Conversation
Codecov Report
@@ Coverage Diff @@
## master #473 +/- ##
==========================================
- Coverage 85.26% 84.63% -0.64%
==========================================
Files 28 28
Lines 794 820 +26
==========================================
+ Hits 677 694 +17
- Misses 117 126 +9
Continue to review full report at Codecov.
|
| @@ -27,3 +28,23 @@ def parse_response(soup): | |||
| print('Mojeek parsed: ' + str(urls)) | |||
|
|
|||
| return urls | |||
|
|
|||
| @staticmethod | |||
| def parse_news_response(soup): | |||
There was a problem hiding this comment.
Just a suggestion, wouldn't it be a better option to just reuse parse_response, why add redundant code
There was a problem hiding this comment.
No for more simplified code, we have to stick to making a new function only.
There was a problem hiding this comment.
We should look into what @dilraj45 said. I don't think it'll make it very complex, here is how I'd go about it
The response parse function while parsing links, attaches a type field to item and returns all results, be it video, news or anything.
Now we just need to filter the ones we need. I think this would be more elegent than creating different function for each type of results we want.
Suggestions ?
There was a problem hiding this comment.
So, this is a condition for a feature to add, or your own concern?
The way you both are suggesting is a complex one and through that way we will increase the complexity https://github.com/fossasia/query-server/blob/master/.travis.yml#LC15 and nothing else.
Secondly, its not necessary that only after making the change that you suggested we will be able to add the feature. Though its working properly in the link https://gentle-basin-55487.herokuapp.com/
If this link is not working properly, then Please let me know.
There was a problem hiding this comment.
I am not saying it's not working, it's about writing good code.
There was a problem hiding this comment.
Please search about the complexity attribute at travis Ci .
We are going to increase flake8 complexity in the manner you are suggesting.
I have tried doing that in Parsijoo Image/Video PR. That's what I am saying.
Additional if else increases complexity in travis
There was a problem hiding this comment.
I agree @bhaveshAn, that adding if-else will increase the cyclomatic complexity, but I guess we need to decide upon an acceptable term between better coding practices and cyclomatic complexity of the code. Feel free to take your call. Additionally, for this particular engine, the implementation of parse_response and parse_news_response is exactly same, so you can at least, instead of rewriting the whole implementation for parse_news_response, just make a call for parse_response in parse_news_response. This will reduce the boilerplate code, plus testing will be easier. Let me know what are your thoughts about this 😄
app/scrapers/__init__.py
Outdated
| @@ -42,6 +42,8 @@ def feed_gen(query, engine, count=10, qtype=''): | |||
| engine = old_names.get(engine, engine) | |||
| if engine in ('quora', 'youtube'): | |||
| urls = scrapers[engine].search_without_count(query) | |||
| elif engine in ('mojeek',) and qtype == 'news': | |||
There was a problem hiding this comment.
Why use in ('mojeek',)
You can use if engine == 'mojeek':
Since there is only one option to choose from
There was a problem hiding this comment.
@gabru-md Made the requested changes 👍
Please review again.
Fixes #472
Addresses #468
Checklist
masterbranch.Changes proposed in this pull request:
test/test_mojeek.py@ParthS007 @vaibhavsingh97 @gabru-md @mariobehling
Providing heroku deployment at https://gentle-basin-55487.herokuapp.com/
Please review. Thanks !!