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

Start updating the documentation comments (QOL) #4689

Merged
merged 16 commits into from
Apr 15, 2024
Merged

Conversation

localden
Copy link
Collaborator

This fix addresses API documentation comments. It's a Quality-of-Life (QOL) improvement that points people to more resources and includes examples, where relevant.

@pmaytak
Copy link
Contributor

pmaytak commented Mar 28, 2024

Is this PR part of bigger set of comment updates? What's in scope?

@localden
Copy link
Collaborator Author

Is this PR part of bigger set of comment updates? What's in scope?

I am cleaning up the comments across the entire library. Every part of MSAL.NET is in scope, but I will make sure that this is limited in number of changes to keep the PR reviewable.

Copy link
Member

@trwalke trwalke left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -5,6 +5,7 @@
using System.ComponentModel;
using System.Runtime.InteropServices;
using Microsoft.Identity.Client.PlatformsCommon.Interfaces;
using static System.Net.WebRequestMethods;
Copy link
Member

Choose a reason for hiding this comment

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

Please undo this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I am actually not sure why this was added. Will remove. Assuming over-eager IntelliSense/IntelliCode.

@@ -75,6 +75,7 @@ private static string GetAuthority()

private static IPublicClientApplication CreatePca(bool withWamBroker = false)
{
// <PCABootstrapSample>
Copy link
Member

Choose a reason for hiding this comment

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

What is this for? Note that this isn't great as a sample, because the token caching is done in a plaintext file.

CHANGELOG.md Show resolved Hide resolved
SECURITY.md Outdated Show resolved Hide resolved
SECURITY.md Outdated Show resolved Hide resolved
@pmaytak pmaytak self-requested a review April 9, 2024 00:05
@pmaytak
Copy link
Contributor

pmaytak commented Apr 11, 2024

@localden Is this ready for review? Should this be set to draft? Seems like there are still additions being done.

@localden
Copy link
Collaborator Author

@pmaytak yep, this should be ready for review. I am done with this batch, and for future PRs will keep them in draft.

@bgavrilMS
Copy link
Member

Can I merge this @localden ?

@localden
Copy link
Collaborator Author

@bgavrilMS absolutely!

localden and others added 10 commits April 15, 2024 18:28
…fication.cs

Co-authored-by: Peter <34331512+pmaytak@users.noreply.github.com>
…ntalTicketManager.cs

Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
…ntalTicketManager.cs

Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
@localden localden requested a review from a team as a code owner April 15, 2024 17:28
@bgavrilMS bgavrilMS merged commit 0876d2b into main Apr 15, 2024
@bgavrilMS bgavrilMS deleted the ddelimarsky/docfix branch April 15, 2024 17:28
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

6 participants