Fixed #4

[zorroblue]

Will research a little more on #2 . This commit fixes #4

[kshitij10496]

@zorroblue

1. I would prefer the check to be placed in search_location (or its helper). Can you explain the reason behind your approach ?
2. This commit attempts to fix 2 issues simultaneously. Please separate the fixes. Also, I would appreciate if you collaborate with @Arafatk over fixing #2.

[zorroblue]

1. I didn't get you here. My approach was pretty simple: in the for loop where you get users from each page, I did an update to reduce max_users after every time I finish off a page. So this sets the limit for the new page. An alternative was introducing a global variable. But I felt it might lead to more changes in the code and make the code lose readability.
2. I should have separated the commits, my mistake :)

[arafatkatze]

This seems reasonable when I check it with

for index in range(10,30):
  users = search_location("Kharagpur", max_users = index)
  print len(users)

Another simple way would be to use a deque and set it maxlen parameter(and break when deque size reaches maxlen). Although I am not sure if its 'better'. Also instead of int(ceil(num))+1 we could also do int(num)+2 and avoid ceil altogether. Just a thought.

[kshitij10496]

Also instead of int(ceil(num))+1 we could also do int(num)+2 and avoid ceil altogether. Just a thought.

This is a PR for separate issue. Let's discuss it at a more suitable place. :smile:

[zorroblue]

@Arafatk @kshitij10496 Am pushing my thoughts for ^^ and ^ over to #2 discussion

[kshitij10496]

Here are a few comments on how to modify this PR:

1. Undo your commit but keep the modified changes
2. Separate the changes into 2 commits.
3. Undo the commit related to #2 (As @Arafatk is working on it)
4. I would prefer to modify the behaviour of search_location function for handling this issue. search_page_users is true to its purpose of returning "list of all the users on a search result page".

If you need help with Git, Stack Overflow is your friend ! If you still can't figure out the procedure, ask on the Gitter Channel.

[kshitij10496]

Ping @zorroblue What's the update on this ?

[zorroblue]

I am really sorry @kshitij10496 I came home now only. Will work and update you by tomorrow :)

[zorroblue]

Hey, I saw #1 fixed. Do I git pull the new source and work on it instead?

[kshitij10496]

@zorroblue Thats absolutely fine ! :smile:

[kshitij10496]

Do this on your master

1. Check your remote first : git remote -v
2. Add this repo as an upstream: git remote add upstream https://github.com/kshitij10496/gh-scrapper.git
3. Fetch the changes and merge: git fetch upstream git merge upstream/master (I don't suggest to use git pull)

Checkout this branch : git checkout limitexceed

1. Rebase this branch onto master: git rebase master
2. If there are Merge commits, then fix them. Google is your friend. :wink:
3. Update your branch to your suiting.
4. Push the changes git push origin limitexceed.

If you face any other problem, comment here or we can discuss it on the Gitter channel.

[zorroblue]

Why don't you suggest git pull ? I thought it was a one-step solution :) Is there something wrong behind it? I have only used it for small projects, never had problems.

[zorroblue]

Hey and Btw I am gonna save your comment in my reference.. You have a great future in StackOverflow,my friend! :D

[kshitij10496]

I have 2 primary reasons for advising against the use of git pull:

1. The problem with git pull is that it has all kinds of helpful "magic" (like you suggested yourself). It means that you don’t really get to learn about the branching in git. Mostly things 'Just Work', but when they don’t it’s often difficult to work out why.
2. The other problem is that by both fetching and merging in one command, your working directory is updated without giving you a chance to examine the changes you’ve just brought into your repository. At times, this can sometimes lead to a difficult situation.

My first ever PR was messed up due to git pull. So there's that as well ! :stuck_out_tongue_winking_eye:

[zorroblue]

@kshitij10496 With regard to the function naming of search_page_users, what you say makes perfect sense. But in that case , why do you pass max_users to the search_page_users function?

[kshitij10496]

But in that case , why do you pass max_users to the search_page_users function?

I don't think that I pass the max_users argument to search_page_users. I am -1 on passing this arg to this functionality.

[zorroblue]

Yeah you are right. I will look into refactoring it in that way. Sounds better :+1:

[kshitij10496]

Cool ! :+1:

[zorroblue]

If _search_page_users has to parse the entire page, how do you think we should target the 'partial scraping' of data?

[zorroblue]

I got a way now :) Ignore ^ My suggestion: Update max_users as I did before..when it falls below 10, trim the users list got from _search_page_users in the _search_location

[kshitij10496]

If _search_page_users has to parse the entire page, how do you think we should target the 'partial scraping' of data?

search_page_users should scrape data from the entire page. search_location need not.

[zorroblue]

@kshitij10496 Got it!

[zorroblue]

@kshitij10496 I did the necessary changes. :)

[zorroblue]

Done!

[zorroblue]

I complicated things a lot. I realised it only after seeing your simple solution.

[kshitij10496]

I complicated things a lot. I realised it only after seeing your simple solution.

No problem !

[zorroblue]

I will correct it asap. I don't remember doing anything with the line spaces or indentation. What could have been the reason behind this??

[kshitij10496]

I would suggest you to read about PEP8 coding style guide. Almost all the Python projects I have come across follow this guide.

It would be better if you can install some plugin for checking the coding style automatically for you.

[kshitij10496]

What could have been the reason?

Are you using Sublime Text as your text editor ?

[zorroblue]

Yes :O

[kshitij10496]

In Sublime Text, the default behaviour when you press Enter in a python module is to auto indent it to 4 spaces(or whatever number you have set. Check your status bar.)

[kshitij10496]

Good work ! This is in. Thanks for your contribution.

[zorroblue]

@kshitij10496 What's next?

[kshitij10496]

@zorroblue Today I am focussing on developing tests for the functions we have implemented as of now.

[zorroblue]

Where can I step in? Can I work on feature 2? Wait, let's chat on Gitter instead!