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.

Sign up for GitHub

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

Jump to bottom

Create README.md #8

Merged
merged 6 commits into from
Dec 12, 2018
Merged

Create README.md #8

merged 6 commits into from
Dec 12, 2018

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Dec 7, 2018 •
edited
Loading

What is in this PR?

  • The structure, especially the second half of this README is derived from ADAL Python's README. I did change all "adal" wording into "msal", and I changed those generic AAD reference docs from v1 to v2.
  • Since the installation steps look simple enough, currently I just put them inline inside the README. (In ADAL, it is a separated page in wiki.) I suspect, as long as the app developers upgrade their pip (which we now emphasized in this README), we might not necessarily need those lengthy installation variations at all.
  • Again, since we have not touched wiki yet, I basically documented what we went through during the latest sync-up meeting, inside the README. In the future we might just add more and more links to each new separated sample repos.

abhidnya13
abhidnya13 approved these changes Dec 11, 2018
View reviewed changes
Copy link
Contributor

@abhidnya13 abhidnya13 left a comment

Choose a reason for hiding this comment

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

LGTM

navyasric
navyasric suggested changes Dec 11, 2018
View reviewed changes
Copy link
Contributor

@navyasric navyasric left a comment

Choose a reason for hiding this comment

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

Thank you @rayluo for starting this. I have added some comments and feedback.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
navyasric
navyasric suggested changes Dec 11, 2018
View reviewed changes
README.md
@@ -0,0 +1,116 @@
# Microsoft Authentication Library (MSAL) for Python

Copy link
Contributor

Choose a reason for hiding this comment

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

As with all our MSAL SDKs, we should add Preview to the title and this note about Preview: https://github.com/AzureAD/microsoft-authentication-library-for-js#important-note-about-the-msal-preview.

In addition, we should probably mention that we have added certain flows and others are yet to come.

Copy link
Collaborator Author

@rayluo rayluo Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I'm adding that preview keyword into the title and adding that preview paragraph.

For the sake of getting this README published sooner (because a short readme is still better than no readme at all), I would suggest we do not mention flows in this version (besides, neither of MSAL .Net or MSAL javascript mentions any flows in their readme). But I totally agree that we would revisit this in (near?) future to give a concise flows coverage.

Copy link

Choose a reason for hiding this comment

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

I believe those details belong in the wiki not in the readme. You can point to a wiki page in the readme which talks about supported flows.

henrik-me
henrik-me approved these changes Dec 11, 2018
View reviewed changes
Copy link

@henrik-me henrik-me 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.

rayluo
rayluo commented Dec 11, 2018
View reviewed changes
Copy link
Collaborator Author

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks for the review! I'm pushing a new commit right now.

README.md
@@ -0,0 +1,116 @@
# Microsoft Authentication Library (MSAL) for Python

Copy link
Collaborator Author

@rayluo rayluo Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I'm adding that preview keyword into the title and adding that preview paragraph.

For the sake of getting this README published sooner (because a short readme is still better than no readme at all), I would suggest we do not mention flows in this version (besides, neither of MSAL .Net or MSAL javascript mentions any flows in their readme). But I totally agree that we would revisit this in (near?) future to give a concise flows coverage.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rayluo rayluo merged commit 5e5f3fc into dev Dec 12, 2018
@rayluo rayluo deleted the readme branch December 12, 2018 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henrik-me henrik-me henrik-me approved these changes

@abhidnya13 abhidnya13 abhidnya13 approved these changes

@navyasric navyasric navyasric approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rayluo @navyasric @abhidnya13 @henrik-me