Boxoffice Guide... #133
Boxoffice Guide... #133
Conversation
There was a problem hiding this comment.
@Pratik-Sanghani Please read the Pep8 guide for whitespacing the expression,
https://www.python.org/dev/peps/pep-0008/?#whitespace-in-expressions-and-statements
And update the PR. Al lot of places have no space between variable, and expressions. Please update that.
Apart from that my only concern is the rate limiting on bookmyshow. Hope we don't end up blocking that person's ID from Bookmyshow itself. Please check on that and update the error message accordingly. We can add a sleep between 2 consecutive bookmyshow requests. A simple Error may not be very helpful.
Apart from that, the logic seems correct.
windows/benji.py
Outdated
| for friend in tweepy.Cursor(api.friends).items(): | ||
| print("\nName: ", json.dumps(friend.name), " Username: ", json.dumps(friend.screen_name)) | ||
|
|
||
| elif len(link[-1]) == "twitter": |
There was a problem hiding this comment.
This is logically not correct len(link[-1]) will give you an integer, which cannot be equal to a string value.
What was the intended change? I would prefer it be reverted to the old if condition.
There was a problem hiding this comment.
Sorry, it was a mistake.
windows/benji.py
Outdated
| #Box Office Status | ||
| elif link[-1] == "boxoffice": | ||
| try: | ||
| url="https://in.bookmyshow.com/"+ link[0] +"/movies/nowshowing" |
There was a problem hiding this comment.
Is there any rate limiting on the bookmyshow.com?
There was a problem hiding this comment.
We should add a disclaimer that Benji assumes the location to be in India.
Book my show works in Indonesia | UAE | Sri Lanka as well.
There was a problem hiding this comment.
I don't think that there is any rate limiting on the bookmyshow.com.
I used only simple web scrapping. And we don't have to login into bookmyshow.com Account.
I'll write that this feature is only available within Indian Region.
windows/benji.py
Outdated
| elif link[-1] == "boxoffice": | ||
| try: | ||
| url="https://in.bookmyshow.com/"+ link[0] +"/movies/nowshowing" | ||
| r= requests.get(url) |
There was a problem hiding this comment.
Ok. Thanks.
|
@Pratik-Sanghani I will review the PR again, once you have addressed the Python styling issue and have the rate-limiting figured out. |
|
Please review the PR and let me know if changes are required. :) @sara-02 |
| for friend in tweepy.Cursor(api.friends).items(): | ||
| print("\nName: ", json.dumps(friend.name), " Username: ", json.dumps(friend.screen_name)) | ||
|
|
||
| elif link[-1] == "twitter": |
There was a problem hiding this comment.
I still don't understand what was wrong with the previous if condition, you are anyway checking the link[-1]==twitter again in the nested if condition.
There was a problem hiding this comment.
previous if condition was giving me Error List Index Out of Range : as per below:
Instead of generating issue , i Solved it.
| end = str(i).index('data-event-code') | ||
| data = str(i)[start+18 : end-2] | ||
|
|
||
| if data == "false": |
There was a problem hiding this comment.
One doubt, when we are hitting the now_showing URL then why do we have a list of coming soon movies? Should we not filter only the ones that are currently running?
Or maybe display the 2 list sepratetly?
There was a problem hiding this comment.
It Displays Movies for which Advance Bookings are Opened. And Releasing on next Weekends.
| if data == "true": | ||
| show_status_list.append("Coming Soon...") | ||
|
|
||
| Tags_list = [] |
There was a problem hiding this comment.
Instead of using 3 lists tags list, name list and show status list, you can create a dictionary of items with the name being the key and the tags being the value, and get rid of the status list altogether
Something like:
{"Now showing": {name: taglist}, {name2:taglist},
"Coming soon": {name3:taglist},{name4:taglist}
}
Also, the tag list for each name should be a set, not a list, t avoid duplicate tags.
Not that lists are wrong, but a dict will be a better presentation of this data. WDYT?
There was a problem hiding this comment.
I can't do it , though status and names are scraped with different levels of the soup.
| speak.runAndWait() | ||
| cntr = len(Name_list) | ||
| print("----------------------------------------------") | ||
| print(link[0].capitalize()) |
There was a problem hiding this comment.
We should differentiate printing the names which are currently showing from the coming soon ones, something like
Print("Now showing")
Print(iterate through dict["Now showing"]
Print("Coming Soon")
Print(iterate through dict["Coming Soon"]
|
|
||
| soup_level2 = [] | ||
| show_status_list = [] | ||
| shows_list = soup.find_all('div', attrs={'class': 'card-container wow fadeIn movie-card-container'}) |
There was a problem hiding this comment.
We can actually get rid of the show_list dict, check the suggestion below
|
@sara-02 are we ready to merge this? |
|
@the-ethan-hunt Yes |
Feature added as per issue #3
Get List of all Movies running in the Cinemas of Town.