Skip to content
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

Fixes #462 Add Video search support for Ask #463

Merged
merged 5 commits into from
Jan 26, 2018

Conversation

bhaveshAn
Copy link
Member

Fixes #462
Addresses #320

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 Video search support for Ask

Providing the heroku deployment at https://salty-spire-70237.herokuapp.com/
@mariobehling @ParthS007 @vaibhavsingh97 Please review. Thanks !!

@codecov
Copy link

codecov bot commented Jan 22, 2018

Codecov Report

Merging #463 into master will decrease coverage by 2.1%.
The diff coverage is 47.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
- Coverage    87.5%   85.39%   -2.11%     
==========================================
  Files          28       28              
  Lines         720      760      +40     
==========================================
+ Hits          630      649      +19     
- Misses         90      111      +21
Impacted Files Coverage Δ
test/test_ask.py 100% <100%> (ø) ⬆️
app/scrapers/ask.py 100% <100%> (ø) ⬆️
app/scrapers/generalized.py 52.22% <4.76%> (-14.45%) ⬇️
app/scrapers/__init__.py 86.66% <50%> (-2.62%) ⬇️

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 bd30392...3ef0ba3. Read the comment docs.

@bhaveshAn bhaveshAn self-assigned this Jan 22, 2018
@bhaveshAn
Copy link
Member Author

Added tests at test/tests_ask.py .
@cclauss @ParthS007 @vaibhavsingh97 @gabru-md Please review.

Copy link
Member

@ParthS007 ParthS007 left a comment

Choose a reason for hiding this comment

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

LGTM ! Please wait for @vaibhavsingh97 review 👍

@bhaveshAn
Copy link
Member Author

@vaibhavsingh97 Please review asap...

Copy link

@rohitsainicool rohitsainicool left a comment

Choose a reason for hiding this comment

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

Great the link working properly 💯

@mariobehling
Copy link
Member

Please resolve conflicts

@@ -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 ('ask',) and qtype == 'vid':
Copy link
Member

Choose a reason for hiding this comment

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

Why use in ('ask',)
You can go with if engine == 'ask'
since there is only one option to choose from?

Copy link
Member Author

Choose a reason for hiding this comment

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

So that other engines can also be added in this tuple, if there implementation is same.

@bhaveshAn bhaveshAn merged commit 26b6f5b into fossasia:master Jan 26, 2018
@bhaveshAn bhaveshAn deleted the ask-video branch January 26, 2018 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Video search support for Ask
6 participants