Skip to content

Provide System.DirectoryServices.Protocols package readme #93518

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
Merged
buyaa-n merged 1 commit into from
Oct 17, 2023
Merged

Provide System.DirectoryServices.Protocols package readme #93518

buyaa-n merged 1 commit into from
Oct 17, 2023

Conversation

@eNeRGy164
Copy link
Contributor

@eNeRGy164 eNeRGy164 commented Oct 14, 2023

Fill out details for the System.DirectoryServices.Protocols NuGet package readme.

See #92228

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-System.DirectoryServices labels Oct 14, 2023
@ghost
Copy link

ghost commented Oct 14, 2023

Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014
See info in area-owners.md if you want to be subscribed.

Issue Details

Fill out details for the System.DirectoryServices.Protocols NuGet package readme.

See #92228

Author: eNeRGy164
Assignees: -
Labels:

area-System.DirectoryServices, community-contribution
Milestone: -

danmoseley
danmoseley reviewed Oct 15, 2023
View reviewed changes
src/libraries/System.DirectoryServices.Protocols/src/PACKAGE.md Outdated
Copy link
Member

@danmoseley danmoseley Oct 15, 2023

Choose a reason for hiding this comment

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

Suggested change
It primarily utilizes the `LdapConnection` type for interacting with LDAP servers, utilizing system native libraries to establish TCP/IP or UDP LDAP connections.
It primarily uses the `LdapConnection` type for interacting with LDAP servers, using system native libraries to establish TCP/IP or UDP LDAP connections.

Pet peeve 😀

src/libraries/System.DirectoryServices.Protocols/src/PACKAGE.md Outdated
Copy link
Member

@danmoseley danmoseley Oct 15, 2023

Choose a reason for hiding this comment

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

Would using be better

Copy link
Contributor Author

@eNeRGy164 eNeRGy164 Oct 15, 2023

Choose a reason for hiding this comment

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

I was doubting about that, this makes it quite explicit that you should dispose, and I think the new way of using without {} is not recognizable enough.

src/libraries/System.DirectoryServices.Protocols/src/PACKAGE.md Outdated
Copy link
Member

@danmoseley danmoseley Oct 15, 2023

Choose a reason for hiding this comment

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

Suggested change
Initially supporting only Windows, the assembly now also supports Unix environments, although not yet achieving feature parity between the two.
Supports Windows and Unix, although some features are not supported on Unix.

Would it be helpful to list some features that aren't available on Unix?

Copy link
Contributor Author

@eNeRGy164 eNeRGy164 Oct 15, 2023

Choose a reason for hiding this comment

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

I have to rely on the source code and documentation, and they are quite sparse. I found some comments on client and server cert options, added that as suggestion.

@danmoseley
Copy link
Member

danmoseley commented Oct 15, 2023

Thanks @eNeRGy164

@eNeRGy164 eNeRGy164 force-pushed the system-directory-services-protocols branch from e44b388 to cb58bb9 Compare October 15, 2023 20:49
danmoseley
danmoseley approved these changes Oct 16, 2023
View reviewed changes
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

Looks good to me but owner of the area should sign off (and confirm code sample)

@danmoseley
Copy link
Member

danmoseley commented Oct 16, 2023

Feel like doing more of the package readmes? Now you got warmed up 😀

@eNeRGy164
Copy link
Contributor Author

eNeRGy164 commented Oct 16, 2023

Feel like doing more of the package readmes? Now you got warmed up 😀

#93485, #93489, #93516 😝

@danmoseley
Copy link
Member

danmoseley commented Oct 16, 2023

Oh, whoops 🙂

@ViktorHofer
Copy link
Member

ViktorHofer commented Oct 17, 2023

@dotnet/area-system-directoryservices please review this package readme PR before EOW so that we can get the change still ported into .NET 8.

@ViktorHofer ViktorHofer mentioned this pull request Oct 17, 2023
Closed
buyaa-n
buyaa-n reviewed Oct 17, 2023
View reviewed changes
buyaa-n
buyaa-n approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@buyaa-n buyaa-n 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 @eNeRGy164, left a comment, overall LGTM

ViktorHofer
ViktorHofer reviewed Oct 17, 2023
View reviewed changes
ViktorHofer
ViktorHofer reviewed Oct 17, 2023
View reviewed changes
@eNeRGy164 eNeRGy164 force-pushed the system-directory-services-protocols branch from cb58bb9 to 2cdb91b Compare October 17, 2023 18:51
@buyaa-n
Copy link
Contributor

buyaa-n commented Oct 17, 2023

The failure unrelated and doesn't seem actionable.

@buyaa-n buyaa-n merged commit fc48802 into dotnet:PackageReadmesContinued Oct 17, 2023
@eNeRGy164 eNeRGy164 deleted the system-directory-services-protocols branch October 18, 2023 05:29
ViktorHofer pushed a commit that referenced this pull request Oct 20, 2023
@eNeRGy164 @ViktorHofer
Provide DirectoryServices.Protocols package readme (#93518)
7ad2ed9
@ghost ghost locked as resolved and limited conversation to collaborators Nov 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ViktorHofer ViktorHofer ViktorHofer left review comments

@danmoseley danmoseley danmoseley approved these changes

+1 more reviewer

@buyaa-n buyaa-n buyaa-n approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.DirectoryServices community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eNeRGy164 @danmoseley @ViktorHofer @buyaa-n