Fixes #456 Add Image/Video search support for Bing #448

bhaveshAn · 2018-01-19T23:53:17Z

Fixes #456
Addresses #320 and #321

Checklist

I have read the Contribution & Best practices Guide and my PR follows them.
My branch is up-to-date with the Upstream master branch.
I have added necessary documentation (if appropriate)

Changes proposed in this pull request:

Added Image Search support for Bing.
Added Video Search support for Bing.

Note Since bing image search and video search respectively at https://www.bing.com/images/search?q=fossasia&FORM=HDRSC2 and https://www.bing.com/videos/search?q=fossasia&FORM=HDRSC3 doesn't support page indexing. Hence, results will come without count, such as we had for Quora and Youtube.

Providing the heroku deployment at https://pacific-wildwood-54349.herokuapp.com/

@mariobehling @vaibhavsingh97 @dgarvit @realslimshanky Please review. Thanks !!

realslimshanky

Please go through the review and also try to re-run Travis build.

realslimshanky · 2018-01-20T01:00:07Z

app/scrapers/bing.py

@@ -30,3 +34,39 @@ def parse_response(self, soup):
        print('Bing parsed: ' + str(urls))

        return urls
+
+    def parse_video_response(self, soup):


This method can be changed to a static method by using @staticmethod and removing self from the position argument.

Since previously it was not useful so not using this time too.

That is a really poor answer. The suggested modification is indeed useful as the linter is trying to help you to avoid unnecessary runtime overhead.

https://www.codacy.com/app/fossasia/query-server/pullRequest?prid=1252888#newIssuesView

Normal: 0.5956953540444374 Class: 0.38477163994684815 Static: 0.18970419897232205 # Three times faster than the normal method call.

#!/usr/bin/env python3 from timeit import timeit setup = """ class TestClass: def add_normal_method(self, i): return i + 1 @classmethod def add_class_method(cls, i): return i + 1 @staticmethod def add_static_method(i): return i + 1 t = TestClass()""" print('Normal:', timeit('t.add_normal_method(1)', setup)) print(' Class:', timeit('t.add_class_method(1)', setup)) print('Static:', timeit('t.add_static_method(1)', setup))

realslimshanky · 2018-01-20T01:00:37Z

app/scrapers/bing.py

+
+        return urls
+
+    def parse_image_response(self, soup):


This method can be changed to a static method by using @staticmethod and removing self from the position argument. Codacy also suggests the same modification.

Since previously it was not useful so not using this time too.

Remorax

The link doesn't seem to work for me. Can you check it once again?

bhaveshAn · 2018-01-20T10:22:23Z

@realslimshanky @Remorax Working fine for me

realslimshanky · 2018-01-20T14:09:51Z

@bhaveshAn code quality is equally as important, also debug from the Travis log. Please look into it.

bhaveshAn · 2018-01-20T15:15:17Z

@realslimshanky

also debug from the Travis log. Please look into it.

Travis is failing in Python3 which has nothing to do with production python version. See at https://github.com/fossasia/query-server/blob/master/runtime.txt
Here we are using python-2.7.14

The log is due to
In app/server.py we are using

10           from app.scrapers import feed_gen, scrapers

and since server.py is already in app directory so I have changed it to

10           from scrapers import feed_gen, scrapers

Note: Locally I wasn't able to run the application due to this bug.

code quality is equally as important,
This method can be changed to a static method by using @staticmethod and removing self from the position argument. Codacy also suggests the same modification.

If you are talking about changed to a static method by using @staticmethod and removing self from the position argument. which codacy is reporting, then do see that we have never following that @staticmethod replace self . If you wanna check than check it out at different scrapers https://github.com/fossasia/query-server/tree/master/app/scrapers
This is the reason I have not used @staticmethod in place of self as we while contributing have to see which approach are we following in current codebase of that project.

realslimshanky · 2018-01-20T16:53:38Z

@bhaveshAn there might be 2 scenarios because of which this might be happening. Either Codacy was added recently for the code quality review or else this feature has been added recently to Codacy. Either way, it's not required for the PR to be merged. We can follow our own convention of defining methods.
I would request @vaibhavsingh97, @mariobehling and other contributors to please help us out here.

bhaveshAn · 2018-01-20T17:37:40Z

@Remorax

there might be 2 scenarios because of which this might be happening. Either Codacy was added recently for the code quality review or else this feature has been added recently to Codacy.

Nope, If you have gone through the PRs you will get that Codacy is the thing which is in this project from too long and specially #319 #318 #287 #240 #230 Here Codacy has failed and still we had added the feature (SEE under DETAILS Section of each PR, after merge)
You can check out that Codacy is the thing we are ignoring whenever there is a feature adding PR.
So, I think no need to emphasize on Codacy.

realslimshanky · 2018-01-20T17:43:05Z

@bhaveshAn okay. this makes it more clear then. Let's only focus on clearing the Travis build.

Remorax · 2018-01-20T18:11:33Z

Um @bhaveshAn I think you tagged me by mistake 😛
Also, in image search can you get the title also like I have done in #447 since we're following the same convention everywhere 😄

…t for Bing

codecov · 2018-01-20T18:53:52Z

Codecov Report

Merging #448 into master will decrease coverage by 1.65%.
The diff coverage is 68.42%.

@@            Coverage Diff            @@
##           master    #448      +/-   ##
=========================================
- Coverage   89.15%   87.5%   -1.66%     
=========================================
  Files          28      28              
  Lines         664     720      +56     
=========================================
+ Hits          592     630      +38     
- Misses         72      90      +18

Impacted Files	Coverage Δ
app/scrapers/bing.py	`100% <100%> (ø)`	⬆️
test/test_bing.py	`100% <100%> (ø)`	⬆️
app/server.py	`84.88% <100%> (+0.17%)`	⬆️
app/scrapers/generalized.py	`66.66% <11.11%> (-19.61%)`	⬇️
app/scrapers/__init__.py	`89.28% <50%> (-6.55%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 423896a...26d8459. Read the comment docs.

bhaveshAn · 2018-01-20T19:02:16Z

@realslimshanky Now you can check Travis as well.

@vaibhavsingh97 @gabru-md @cclauss Please review

cclauss · 2018-01-20T19:58:01Z

Why this persist in clinging to legacy Python?

Also the argument that is a good idea to make our code slow for "code consistency" reasons is quite weak.

cclauss

Ignores good performance suggestions.

bhaveshAn · 2018-01-22T08:40:37Z

@realslimshanky @cclauss @vaibhavsingh97 Please review.
Codacy and Travis CI both are passing their tests.

realslimshanky

Please squash your commits.

realslimshanky · 2018-01-22T09:12:41Z

package-lock.json

@@ -0,0 +1,51 @@
+{


Please remove this generated file from the PR, adding it to .gitignore would be a good option.

Check out https://medium.com/@Quigley_Ja/everything-you-wanted-to-know-about-package-lock-json-b81911aa8ab8#1c9a
Require to commit package-lock.json

Since this file hasn't been added to the repository yet, it should be segregated from the current issue which is trying to add Image/Video support. Let's create a separate issue for the package-lock.json.

realslimshanky · 2018-01-22T09:16:00Z

app/scrapers/__init__.py

@@ -42,6 +42,10 @@ 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 ['bing']) and (qtype == 'vid'):


It would be helpful if we follow the same convention for conditional statements. As line 43 uses a tuple, we should use tuple for line 45 and 47 as well, i.e. changing engine in ['bing'] to engine in ('bing').

elif engine in ['bing'] and qtype == 'vid': # Lose the extra parens -- this is not JavaScript

I was talking about the use of list and tuple in the conditional statements. We should follow one convention to reduce confusion and better readability.

cclauss · 2018-01-21T10:05:11Z

app/server.py

    from scrapers import feed_gen, scrapers
+except Exception as e:


This seems to be going in the wrong direction. From PEP8: "When catching exceptions, mention specific exceptions whenever possible". Also, why create an unused variable (e)? Linters will properly complain.

cclauss · 2018-01-22T09:22:39Z

app/scrapers/__init__.py

@@ -42,6 +42,10 @@ 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 ['bing']) and (qtype == 'vid'):


elif engine in ['bing'] and qtype == 'vid': # Lose the extra parens -- this is not JavaScript

bhaveshAn · 2018-01-22T10:23:02Z

@realslimshanky @cclauss Now you can approve PR.
Will squash commits after approval.

cclauss · 2018-01-22T10:30:19Z

app/server.py

@@ -80,7 +80,8 @@ def search(search_engine):
            unicode  # unicode is undefined in Python 3 so NameError is raised
            for line in result:
                line['link'] = line['link'].encode('utf-8')
-                line['title'] = line['title'].encode('utf-8')
+                if 'desc' in line:


Should be 'title', not 'desc'?

bhaveshAn · 2018-01-22T10:59:46Z

@mariobehling Please review and merge. Thanks!!

ParthS007

@bhaveshAn Please check why your PR is decreasing Code coverage?

bhaveshAn · 2018-01-22T11:29:41Z

@ParthS007
Its because for every feature based PR, there is no test in test directory. Not relevant to this PR.
Check here https://github.com/fossasia/query-server/blob/master/test/test_bing.py
Raising new issue with this thing might help.

rohitsainicool

Awesome 👍

mariobehling · 2018-01-23T10:18:44Z

Please resolve conflicts.

bhaveshAn · 2018-01-23T10:21:33Z

Done @mariobehling

bhaveshAn · 2018-01-23T18:58:29Z

@ParthS007 Now I have added the tests in test/test_bing.py and in overall its not decreasing the coverage.
@mariobehling Please review. Thanks !!

ParthS007

LGTM ! Please squash the commits 👍

bhaveshAn · 2018-01-25T20:53:02Z

@ParthS007 Please approve the PR.

realslimshanky suggested changes Jan 20, 2018

View reviewed changes

Remorax reviewed Jan 20, 2018

View reviewed changes

bhaveshAn self-assigned this Jan 20, 2018

bhaveshAn added the needs-review label Jan 20, 2018

Addresses fossasia#320 and fossasia#321 Add Image/Video search suppor…

946781a

…t for Bing

bhaveshAn force-pushed the bing branch from b6849c5 to 946781a Compare January 20, 2018 18:53

cclauss suggested changes Jan 20, 2018

View reviewed changes

Resolve conflicts

4cda2e0

bhaveshAn changed the title ~~Addresses #320 and #321 Add Image/Video search support for Bing~~ Fixes #456 Add Image/Video search support for Bing Jan 21, 2018

bhaveshAn added 2 commits January 22, 2018 14:02

Merge remote-tracking branch 'upsteam/master' into bing

42a36ce

Add staticmethod decorator

e54dbbb

realslimshanky reviewed Jan 22, 2018

View reviewed changes

cclauss suggested changes Jan 22, 2018

View reviewed changes

Add staticmethod decorator

978015d

cclauss approved these changes Jan 22, 2018

View reviewed changes

cclauss reviewed Jan 22, 2018

View reviewed changes

bhaveshAn added 2 commits January 22, 2018 16:01

Add staticmethod decorator

5c009be

Add staticmethod decorator

15ba9b3

realslimshanky approved these changes Jan 22, 2018

View reviewed changes

ParthS007 reviewed Jan 22, 2018

View reviewed changes

rohitsainicool approved these changes Jan 22, 2018

View reviewed changes

Resolved conflicts

da47877

bhaveshAn added 5 commits January 24, 2018 00:07

Added tests

e16ebfe

Added tests

81c3de7

Added tests

da58033

Added tests

8c9f058

Added tests

26d8459

bhaveshAn removed the needs-review label Jan 24, 2018

ParthS007 reviewed Jan 25, 2018

View reviewed changes

mariobehling merged commit bd30392 into fossasia:master Jan 26, 2018

bhaveshAn deleted the bing branch January 26, 2018 14:11

		from scrapers import feed_gen, scrapers
		except Exception as e:

Fixes #456 Add Image/Video search support for Bing #448

Fixes #456 Add Image/Video search support for Bing #448

Conversation

bhaveshAn commented Jan 19, 2018 • edited Loading

Checklist

Changes proposed in this pull request:

realslimshanky left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cclauss Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remorax left a comment

Choose a reason for hiding this comment

bhaveshAn commented Jan 20, 2018

realslimshanky commented Jan 20, 2018

bhaveshAn commented Jan 20, 2018

realslimshanky commented Jan 20, 2018

bhaveshAn commented Jan 20, 2018

realslimshanky commented Jan 20, 2018

Remorax commented Jan 20, 2018

codecov bot commented Jan 20, 2018 • edited Loading

Codecov Report

bhaveshAn commented Jan 20, 2018

cclauss commented Jan 20, 2018

cclauss left a comment

Choose a reason for hiding this comment

bhaveshAn commented Jan 22, 2018

realslimshanky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhaveshAn commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhaveshAn commented Jan 22, 2018

ParthS007 left a comment

Choose a reason for hiding this comment

bhaveshAn commented Jan 22, 2018

rohitsainicool left a comment

Choose a reason for hiding this comment

mariobehling commented Jan 23, 2018

bhaveshAn commented Jan 23, 2018

bhaveshAn commented Jan 23, 2018

ParthS007 left a comment

Choose a reason for hiding this comment

bhaveshAn commented Jan 25, 2018

bhaveshAn commented Jan 19, 2018 •

edited

Loading

realslimshanky left a comment •

edited

Loading

cclauss Jan 20, 2018 •

edited

Loading

codecov bot commented Jan 20, 2018 •

edited

Loading