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

search_people using the Keywords filter #97

Merged
merged 3 commits into from Jun 25, 2020
Merged

search_people using the Keywords filter #97

merged 3 commits into from Jun 25, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jun 23, 2020

Hi, small PR here :

  1. I added the possibility to search people using the Keywords filters from LinkedIn which are super helpful (see screenshot below)

Screenshot from 2020-06-23 14-42-53

  1. I fixed what I think was an issue where limit could be None which would result in a TypeError: unsupported operand type(s) for -: 'NoneType' and 'int' error (please review this change to make sure I didn't break the previous logic even though I don't think so)

@ghost
Copy link
Author

ghost commented Jun 23, 2020

There's now a duplicated parameter : title and keyword_title. I thought that having the keyword_ syntax made sense. However, the duplicated params could lead to confusion. We can either :

  • remove the extra keyword_title parameter
  • remove the title parameter (would be a breaking change for some users though)
  • let both and then maybe do somethin like title = title if title else keyowrd_title

@tomquirk
Copy link
Owner

@crouinicrouina this is fantastic, thank you for the contribution 🥳 . I'm happy to merge this.

Regarding the title/keyword_title issue, I agree keyword_title makes sense, but due to the breaking nature of the change, I would prefer if we did the 3rd option.

I also think a comment next to to the parameters clearly stating that they do the same thing would be helpful as well.

Ping me when you're ready for merge!

@ghost
Copy link
Author

ghost commented Jun 24, 2020

Hey @tomquirk, as you suggested, I went with the 3rd option and clarified the parameters usage directly in the code (since these parameters are not documented yet in DOCS.md.

I think this is ready for merge now :)

I have a question regarding PR best practices in git (I'm new to git collaboration). Which is better :

  • Create a PR for separate issues (i.e: 1 to 1 mapping with an exception for duplicated issues)
  • Create a PR for multiple issues at once. For instance, after making this PR, I bumped into a bug : search encoding error on the keywords query string parameter #98 and I'm not sure if I should rather fix this bug in this PR or create a new branch in my forked repository and then create a second PR for this issue (especially since I created this PR prior to finding the bug) ?

@tomquirk
Copy link
Owner

@crouinicrouina awesome! Thank you for adding those comments; super helpful. I am happy to merge this! 🎉

Regarding your question: in my opinion it's almost always better to opt for creating PR for separate issues, because smaller PRs:

  • are easier to review
  • can be reverted easier
  • are less risky because many things aren't being changed at once
  • can be easily referred back to in the future, if someone is seeking additional context for an issue

Again, I really appreciate your contribution here and your comments in some of the issues. Glad to have you involved in the project!

@tomquirk tomquirk merged commit 56f46e9 into tomquirk:master Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant