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

Documentation #204

Merged
merged 14 commits into from
Jan 27, 2014
Merged

Documentation #204

merged 14 commits into from
Jan 27, 2014

Conversation

hundredwatt
Copy link
Collaborator

This is for #192

This is not quite ready to be merged, but it's very close, so I figured I'd go ahead and open a PR to start the discussion.

I switched from rdoc to yard as the documentation engine. I then went through and documented all the API methods and other classes. Finally, I re-organized the API module into submodules based on how LinkedIn organizes the APIs in their documentation. I also made a number of small changes and additions to help clean up and complete the documentation.

Further, I went through all the LinkedIn REST API documentation and added a note at the top of each submodule with any endpoints that are missing from this library.

Here are the remaining questions I had:

  1. Hyphenated Query Parameters
    The LinkedIn API uses hyphenated names for multi-word parameters. The Ruby convention is to use underscores in symbol names, so that's what I did in the documentation. Will the Client automatically convert underscore to hyphenated when the requests are made or should I go back and change the documentation to hyphenated names?

Example: "event-type" for company updates.
Here: https://github.com/hundredwatt/linkedin/blob/docs/lib/linked_in/api/companies.rb#L39
vs
Here: http://developer.linkedin.com/reading-company-shares

  1. x-li-auth-token Header
    The x-li-auth-token header is used to access out of network profiles: http://developer.linkedin.com/documents/accessing-out-network-profiles

It doesn't look like it's currently possible to add this header to a #profile request, is that correct?
Assuming so, I wanted to get thoughts on addressing this in documentation and/or modifying the code to allow setting this header? Or perhaps this is something that should wait for #187 and switching to Faraday?

A couple other nice to haves. Should I open these as separate issues?

  1. Instead of having the #search method take a type and live in the Search module, I think it should be split into 3 separate methods and then place them in the appropriate modules: #people_search, #company_search, #job_search
  2. For #send_message in the Communications API, should we use the person_path helper instead of asking the user to supply an array of person_paths? https://github.com/hundredwatt/linkedin/blob/docs/lib/linked_in/api/communications.rb#L34. I haven't personally used the Communications API, so I'm not sure what's best here.

Let me know if there are any questions/comments/feedback on any of the commits in my docs branch

@hundredwatt
Copy link
Collaborator Author

I also marked a few issues in my code with 'TODO' to indicate I needed to come back to them. Here are my questions for those:

  1. Do #group_profile and #group_posts require the rw_groups permission?
  2. Do #network_updates, #share_comments, and #share_likes require the rw_nus permission?
  3. Do we need the #shares method in API::ShareAndSocialStream? It seems redundant to the #network_updates method
  4. The person_path helper can take an "email" attribute. I could not find anything in LinkedIn's documentation about this. Is it working and undocumented or not working
  5. Same issue with company_path helper's url attribute.

Also, I'm not sure about #picture_urls, see #201

@hexgnu
Copy link
Owner

hexgnu commented Jan 21, 2014

💥 holy hell this is awesome

@hexgnu
Copy link
Owner

hexgnu commented Jan 21, 2014

For the hyphenated params yea I don't think they get automatically converted. In most cases with this gem you have to do something like :"x-a-b-c" => 'foobaz' instead of relying on underscores. I feel we should address that soon and will make sure we reconvene on that.

I totally agree with breaking out search into other methods. I'd recommend that we put a warning message on that particular endpoint and then deprecate it when we bump the version up to 0.5.x

As for the send_message honestly I have relied on other people for PR's on this one. I don't use it either.

Feel free to open issues and we can attack them one by one.

For a nice example on using this in a [Rails App](http://pivotallabs.com/users/will/blog/articles/1096-linkedin-gem-for-a-web-app).

If you want to play with the LinkedIn api without using the gem, have a look at the [apigee LinkedIn console](http://app.apigee.com/console/linkedin).

## Examples

More examples are in the [examples folder](http://github.com/pengwynn/linkedin/blob/master/examples).
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't notice this before but the link should be 'https://github.com/hexgnu/linkedin/blob/master/examples' since Wynn turned over dev to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done: 3f097a2

@hexgnu
Copy link
Owner

hexgnu commented Jan 21, 2014

Do #group_profile and #group_posts require the rw_groups permission?

I believe so

Do #network_updates, #share_comments, and #share_likes require the rw_nus permission?

Yes

Do we need the #shares method in API::ShareAndSocialStream? It seems redundant to the #network_updates method

It might be completely redundant. We can make a task for that

The person_path helper can take an "email" attribute. I could not find anything in LinkedIn's documentation about this. Is it working and undocumented or not working

Same issue with company_path helper's url attribute.

There were some PR's recently uploaded that included that stuff that honestly has to do with the 'preferred' API. I don't have access, and don't know anybody who does. I've also tried to reach out to linkedin but they aren't very helpful. I think we should try again.

@hexgnu
Copy link
Owner

hexgnu commented Jan 21, 2014

I think that it might be a good idea to bring all of the examples into one file instead of having multiple ruby / markdown pages. That way we can access it all via YARD.

This is seriously awesome thanks!

hundredwatt added a commit to hundredwatt/linkedin that referenced this pull request Jan 21, 2014
@hundredwatt
Copy link
Collaborator Author

@hexgnu Great feedback and thanks for addressing all my questions 😄

I'm going go through all of the points we discussed and create a todolist in this issue with the tasks that should be finished before merging. I'll then open up issues for other tasks that don't need to be included in this PR.

hundredwatt added a commit to hundredwatt/linkedin that referenced this pull request Jan 23, 2014
hundredwatt added a commit to hundredwatt/linkedin that referenced this pull request Jan 23, 2014
hundredwatt added a commit to hundredwatt/linkedin that referenced this pull request Jan 23, 2014
@hundredwatt
Copy link
Collaborator Author

Here's a summary of remaining tasks from this pull request

Tasks to be complete before merging:

  • Bring all examples into 1 file. Also, add examples to relevant documentation sections
  • Run rebase to squash this down to fewer commits

Tasks to be moved to other issues:

  • Support x-li-auth-token
  • Deprecate #search method and instead create 3 separate methods for people, company, and jobs search
  • Use #person_path_helper for the #send_message method
  • For the #person_path helper's "email" attribute and the #company_path helper's url attribute, find documentation for this (suspected to be from premium API)
  • Automatically convert underscored query parameters to hyphenated
  • Re-organize spec/ folder to match new module hierarchy

@hundredwatt
Copy link
Collaborator Author

@hexgnu I'll finish off the last task for this PR by the end of the weekend. Any other things we should address before merging?

hundredwatt added a commit to hundredwatt/linkedin that referenced this pull request Jan 23, 2014
  * Add link to CHANGELOG in README
  * Update examples link. Relates to hexgnu#204
  * JSON API is now being used, so remove TODO
  * Add section in README about documentation
Match organization in LinkedIn's REST API Documentation: http://developer.linkedin.com/rest
Clarify #add_job_bookmark argument and set convention of using 'update_key' as the argument name
@hundredwatt
Copy link
Collaborator Author

I squashed 43 commits down to 10

@hundredwatt
Copy link
Collaborator Author

@hexgnu This is ready to go from my perspective! Do you have any other comments before we merge?

hexgnu pushed a commit that referenced this pull request Jan 27, 2014
@hexgnu hexgnu merged commit 2a38843 into hexgnu:master Jan 27, 2014
@hexgnu
Copy link
Owner

hexgnu commented Jan 27, 2014

🍺 great work @hundredwatt

@hundredwatt hundredwatt mentioned this pull request Nov 22, 2014
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.

2 participants