Fixes #455 Add Image/Video search support for Parsijoo #449

bhaveshAn · 2018-01-20T12:46:18Z

Fixes #455
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 Parsijoo
Added Video search support for Parsijoo
Travis CI build : https://travis-ci.org/bhaveshAn/query-server/builds/331264414

Providing heroku deployment at https://hidden-forest-11970.herokuapp.com/

@mariobehling @vaibhavsingh97 @ParthS007 Please review. Thanks !!

codecov · 2018-01-20T18:30:46Z

Codecov Report

Merging #449 into master will decrease coverage by 0.25%.
The diff coverage is 75.55%.

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   85.26%   85.01%   -0.26%     
==========================================
  Files          28       28              
  Lines         794      834      +40     
==========================================
+ Hits          677      709      +32     
- Misses        117      125       +8

Impacted Files	Coverage Δ
app/scrapers/generalized.py	`48.18% <0%> (-3.78%)`	⬇️
test/test_parsijoo.py	`100% <100%> (ø)`	⬆️
app/scrapers/__init__.py	`86.66% <100%> (ø)`	⬆️
app/scrapers/parsijoo.py	`100% <100%> (ø)`	⬆️

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 659dee4...8d328d9. Read the comment docs.

…t for Parsijoo

cclauss · 2018-01-21T00:15:51Z

.travis.yml

@@ -9,7 +9,7 @@ install:
  - pip install -r requirements.txt

 before_script:
-  - flake8 . --count --max-complexity=15 --show-source --statistics
+  - flake8 . --count --max-complexity=16 --show-source --statistics --max-line-length=100


Why do we need to go back to violating PEP8 line length?

cclauss

Can we refactor to reduce McCabe complexity?

cclauss · 2018-01-21T10:06:44Z

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.

bhaveshAn · 2018-01-22T08:36:02Z

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

realslimshanky · 2018-01-22T09:07:42Z

For some reason, video URL is not working.

cclauss · 2018-01-22T09:14:26Z

app/scrapers/generalized.py

+        if urls == []:
+            return "No video with this Keyword"
+        else:
+            return urls


Could we just do return urls or "No video with this Keyword" because all other falsey values are just as dangerous as an empty list.

cclauss · 2018-01-22T09:17:12Z

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 ['parsijoo']) and (qtype == 'isch'):


elif engine in ['parsijoo'] and qtype == 'isch': # Lose the parens... this is not JavaScript.

cclauss · 2018-01-22T09:45:37Z

app/scrapers/__init__.py

@@ -42,9 +42,9 @@ 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 ['parsijoo']) and (qtype == 'isch'):
+    elif engine in ('parsijoo') and qtype == 'isch':


('parsijoo') is not a tuple, it is a str. ('parsijoo', ) is a tuple. However the test still works as expected.

$ python3 -c "print(type(('parsijoo')), type(('parsijoo', )))"

cclauss · 2018-01-22T09:56:18Z

app/scrapers/parsijoo.py

+                    [[url1], [url2], ...]
+        """
+        urls = []
+        for div in soup.findAll('div', attrs={'class': 'image-container overflow'}):


for div in soup.find_all('div', class_='image-container overflow'): # for PEP8 compatibility

bhaveshAn · 2018-01-22T10:19:35Z

@realslimshanky @cclauss I think now you can approve the PR.

realslimshanky · 2018-01-22T11:01:12Z

@bhaveshAn please squash your commits.

bhaveshAn · 2018-01-22T11:04:07Z

Will squash after final approvals of PRs.

cclauss

flake8 remove --max-line-length=100

bhaveshAn · 2018-01-22T11:10:09Z

app/scrapers/parsijoo.py

+        """
+        urls = []
+        for div in \
+                soup.findAll('div', attrs={'class': 'image-container overflow'}):


@cclauss

flake8 remove --max-line-length=100

Can't be done to for this reason. Its exceeding the default max-line-length

Please reformat as for div in soup.find_all('div', class_='image-container overflow'): to avoid the backslash and the long line. Both are advised against in PEP8.

ParthS007

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

bhaveshAn · 2018-01-22T11:28:33Z

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_parsijoo.py
Raising and new issue with this thing might help.

mariobehling · 2018-01-23T10:17:55Z

Please resolve conflicts.

bhaveshAn · 2018-01-23T10:26:49Z

Done @mariobehling

cclauss · 2018-01-23T10:36:45Z

Two PEP8 violations in this PR can be avoided with a simple one line change.

https://github.com/fossasia/query-server/pull/449/files#r162910342

The modifications to .travis.yml will also become unnecessary.

bhaveshAn · 2018-01-23T19:19:58Z

@ParthS007 Now, I have also added tests respectively for Image/Video search in test/tests_parsijoo.py
Please review @mariobehling Thanks!!

bhaveshAn · 2018-01-23T19:47:57Z

@cclauss Added tests at test/tests_parsijoo.py please review. Thanks!!

ParthS007

LGTM ! Please squash the commits 👍

bhaveshAn · 2018-01-25T20:52:43Z

@ParthS007 Please approve the pull request

cclauss · 2018-01-25T20:52:49Z

My comments on --max-line-length=100 stand but perhaps others disagree with me.

bhaveshAn · 2018-01-26T12:47:38Z

@ParthS007 @vaibhavsingh97 @mariobehling Please review

bhaveshAn added the needs-review label Jan 20, 2018

bhaveshAn self-assigned this Jan 20, 2018

bhaveshAn force-pushed the parsijoo-image branch from 62e7735 to aa55eca Compare January 20, 2018 18:30

bhaveshAn force-pushed the parsijoo-image branch from aa55eca to 5b60b9f Compare January 20, 2018 18:34

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

70899f8

…t for Parsijoo

bhaveshAn force-pushed the parsijoo-image branch from 5b60b9f to 70899f8 Compare January 20, 2018 18:38

cclauss reviewed Jan 21, 2018

View reviewed changes

cclauss suggested changes Jan 21, 2018

View reviewed changes

Resolve conflicts

afc397d

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

cclauss suggested changes Jan 21, 2018

View reviewed changes

bhaveshAn added 4 commits January 22, 2018 13:11

Merge remote-tracking branch 'upstream/master' into parsijoo-image

6ff5499

Add staticmethod decorator

ea4618f

Add staticmethod decorator

5692c94

Add staticmethod decorator

e80eaeb

Add staticmethod decorator

7c17a89

cclauss reviewed Jan 22, 2018

View reviewed changes

bhaveshAn added 2 commits January 22, 2018 15:06

Add staticmethod decorator

5251491

Add staticmethod decorator

cea1408

cclauss reviewed Jan 22, 2018

View reviewed changes

Add staticmethod decorator

c5b0397

cclauss reviewed Jan 22, 2018

View reviewed changes

bhaveshAn added 2 commits January 22, 2018 15:42

Add staticmethod decorator

8f47338

Add staticmethod decorator

40e5246

Add staticmethod decorator

1e865a9

cclauss suggested changes Jan 22, 2018

View reviewed changes

bhaveshAn commented Jan 22, 2018

View reviewed changes

ParthS007 suggested changes Jan 22, 2018

View reviewed changes

rohitsainicool approved these changes Jan 22, 2018

View reviewed changes

Resolved conflicts

3df4a23

Add tests

c37af57

ParthS007 reviewed Jan 25, 2018

View reviewed changes

Resolved conflicts

ac01b4b

Merge branch 'master' into parsijoo-image

d45d132

shankybora approved these changes Jan 28, 2018

View reviewed changes

Merge branch 'master' into parsijoo-image

8d328d9

realslimshanky approved these changes Jan 28, 2018

View reviewed changes

mariobehling merged commit 70a7e96 into fossasia:master Jan 29, 2018

bhaveshAn deleted the parsijoo-image branch January 29, 2018 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #455 Add Image/Video search support for Parsijoo #449

Fixes #455 Add Image/Video search support for Parsijoo #449

bhaveshAn commented Jan 20, 2018 •

edited

Loading

codecov bot commented Jan 20, 2018 •

edited

Loading

cclauss Jan 21, 2018

cclauss left a comment

cclauss Jan 21, 2018

bhaveshAn commented Jan 22, 2018

realslimshanky commented Jan 22, 2018

cclauss Jan 22, 2018

cclauss Jan 22, 2018

cclauss Jan 22, 2018

cclauss Jan 22, 2018

bhaveshAn commented Jan 22, 2018

realslimshanky commented Jan 22, 2018

bhaveshAn commented Jan 22, 2018

cclauss left a comment

bhaveshAn Jan 22, 2018

cclauss Jan 22, 2018 •

edited

Loading

ParthS007 left a comment

bhaveshAn commented Jan 22, 2018

mariobehling commented Jan 23, 2018

bhaveshAn commented Jan 23, 2018

cclauss commented Jan 23, 2018

bhaveshAn commented Jan 23, 2018

bhaveshAn commented Jan 23, 2018

ParthS007 left a comment

bhaveshAn commented Jan 25, 2018

cclauss commented Jan 25, 2018

bhaveshAn commented Jan 26, 2018

		from scrapers import feed_gen, scrapers
		except Exception as e:

Fixes #455 Add Image/Video search support for Parsijoo #449

Fixes #455 Add Image/Video search support for Parsijoo #449

Conversation

bhaveshAn commented Jan 20, 2018 • edited Loading

Checklist

Changes proposed in this pull request:

codecov bot commented Jan 20, 2018 • edited Loading

Codecov Report

cclauss Jan 21, 2018

Choose a reason for hiding this comment

cclauss left a comment

Choose a reason for hiding this comment

cclauss Jan 21, 2018

Choose a reason for hiding this comment

bhaveshAn commented Jan 22, 2018

realslimshanky commented Jan 22, 2018

cclauss Jan 22, 2018

Choose a reason for hiding this comment

cclauss Jan 22, 2018

Choose a reason for hiding this comment

cclauss Jan 22, 2018

Choose a reason for hiding this comment

cclauss Jan 22, 2018

Choose a reason for hiding this comment

bhaveshAn commented Jan 22, 2018

realslimshanky commented Jan 22, 2018

bhaveshAn commented Jan 22, 2018

cclauss left a comment

Choose a reason for hiding this comment

bhaveshAn Jan 22, 2018

Choose a reason for hiding this comment

cclauss Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

ParthS007 left a comment

Choose a reason for hiding this comment

bhaveshAn commented Jan 22, 2018

mariobehling commented Jan 23, 2018

bhaveshAn commented Jan 23, 2018

cclauss 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

cclauss commented Jan 25, 2018

bhaveshAn commented Jan 26, 2018

bhaveshAn commented Jan 20, 2018 •

edited

Loading

codecov bot commented Jan 20, 2018 •

edited

Loading

cclauss Jan 22, 2018 •

edited

Loading