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

Linkedin compliance removal & LinkedIn Example update/fix #397

Merged
merged 7 commits into from
Feb 17, 2020

Conversation

jtroussard
Copy link
Contributor

This PR is an extension/revision of PR #388 - Upon further review and testing it has been determined that LinkedIn's API as it stands is in compliance and the compliance fixes were introducing errors in certain scenarios related to appending token codes to resource request URLs.

In short this PR includes:

  • All LinkedIn compliance fix code, and test code has been removed.
  • Updates LinkedIn Example
    • Updated links
    • Updated python print statements to use python 3 convention and format string method
    • Removed importing, registering, and implementation of compliance fixes.
  • Updated History file

@jtroussard jtroussard added bug clean-up Remove dead code. Formatting. Whitespace/readability. labels Feb 17, 2020
@coveralls
Copy link

coveralls commented Feb 17, 2020

Coverage Status

Coverage decreased (-0.3%) to 90.164% when pulling feafa6d on linkedin-compliance-removal into 29ba9af on master.

Copy link
Member

@singingwolfboy singingwolfboy left a comment

Choose a reason for hiding this comment

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

Looks great overall! Just a few minor suggestions

docs/examples/linkedin.rst Outdated Show resolved Hide resolved

>>> # Fetch the access token
>>> linkedin.fetch_token(token_url, client_secret=client_secret,
... authorization_response=redirect_response)
>>> linkedin.fetch_token(token_url,client_secret=client_secret,include_client_id=True,authorization_response=redirect_response)
Copy link
Member

Choose a reason for hiding this comment

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

Can you break this across multiple lines? It's hard to read all on one line like this.

Copy link
Contributor Author

@jtroussard jtroussard Feb 17, 2020

Choose a reason for hiding this comment

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

No problem. This file as a whole is weird to me. Not sure why it was made into an .rst, and even if it's something the OC wanted documented this is sort of a pain to edit. Perhaps as a back log item for this could be to reformat these examples so that they render well in documentation yet are more user friendly e.g. pull down and hit play.

jtroussard and others added 3 commits February 17, 2020 15:45
First I'm seeing f strings. I've always been happy with format method, but I'll check the docs and give these a whirl.

Co-Authored-By: David Baumgold <[email protected]>

>>> # Fetch the access token
>>> linkedin.fetch_token(token_url, client_secret=client_secret,
>>> linkedin.fetch_token(token_url,client_secret=client_secret,
Copy link
Member

Choose a reason for hiding this comment

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

More more nitpick: you dropped a space here, which makes it less readable

Copy link
Member

@singingwolfboy singingwolfboy left a comment

Choose a reason for hiding this comment

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

Looks great to me! Feel free to merge whenever you're ready.

@jtroussard jtroussard merged commit d75279c into master Feb 17, 2020
@jtroussard jtroussard deleted the linkedin-compliance-removal branch February 17, 2020 21:05
jtroussard pushed a commit that referenced this pull request Feb 21, 2022
…oval"

This reverts commit d75279c, reversing
changes made to 29ba9af.
jtroussard pushed a commit that referenced this pull request Feb 28, 2022
…oval"

This reverts commit d75279c, reversing
changes made to 29ba9af.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug clean-up Remove dead code. Formatting. Whitespace/readability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants