Merge lp://qastaging/~jpds/friends/linkedin-protocol into lp://qastaging/~super-friends/friends/trunk-next

Proposed by Jonathan Davies
Status: Rejected
Rejected by: Robert Bruce Park
Proposed branch: lp://qastaging/~jpds/friends/linkedin-protocol
Merge into: lp://qastaging/~super-friends/friends/trunk-next
Diff against target: 200 lines (+170/-2)
4 files modified
debian/changelog (+6/-2)
debian/control (+6/-0)
debian/friends-linkedin.install (+1/-0)
friends/protocols/linkedin.py (+157/-0)
To merge this branch: bzr merge lp://qastaging/~jpds/friends/linkedin-protocol
Reviewer Review Type Date Requested Status
Robert Bruce Park Disapprove
Review via email: mp+155195@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-03-25.

Description of the change

Implemented a base LinkedIn protocol for friends.

To post a comment you must log in.
Revision history for this message
Robert Bruce Park (robru) wrote : Posted in a previous version of this proposal

MP into wrong branch ;-)

review: Needs Resubmitting
Revision history for this message
Robert Bruce Park (robru) wrote :

Ok, looks mostly good. You're missing test coverage, however. Please see tests/test_twitter.py to see how we mock away everything and test just the methods you wrote. Also, please include a raw JSON dump from the server so that we know things are actually working ;-)

Other than that, just some small nitpicks:

* person.get('siteStandardProfileRequest').get('url')

Code like that should really be like this instead:

* person.get('siteStandardProfileRequest', {}).get('url', '')

And generally all calls to .get() should provide a default value. This allows the plugin to be robust and still publish something in the event that some of the data is missing for whatever unpredictable reason.

Also, you have this comment:

# Posts gives us a likes dict, while replies give us an int.

Although the code that follows it does not agree with what is written in the comment. Is that just a copy&paste error from the Facebook plugin? If so, can you delete that comment? ;-)

Thanks Jonathan!

review: Needs Fixing
Revision history for this message
Robert Bruce Park (robru) wrote :

Also, it looks like your contacts implementation is incomplete. It downloads the contacts (which I saw in the debugging output), but it doesn't call self._push_to_eds in order to actually save those contacts (so they're just downloaded and then discarded currently).

197. By Jonathan Davies

Gave url .get() a default value for robustness.

198. By Jonathan Davies

Save LinkedIn connections to EDS with _push_to_eds().

Revision history for this message
Robert Bruce Park (robru) wrote :

Any word on test coverage for this, Jon?

Revision history for this message
Robert Bruce Park (robru) wrote :
review: Disapprove

Unmerged revisions

198. By Jonathan Davies

Save LinkedIn connections to EDS with _push_to_eds().

197. By Jonathan Davies

Gave url .get() a default value for robustness.

196. By Jonathan Davies

Leave comment about private profiles.

195. By Jonathan Davies

Start of LinkedIn contacts syncing.

194. By Jonathan Davies

Beginnings of friends-linkedin package.

193. By Jonathan Davies

Foundations of our LinkedIn network updates gathering.

192. By Jonathan Davies

Start of linkedin.py protocol for Friends.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: