Skip to content

Conversation

@dhivyada
Copy link

Updates README.md

Copy link
Contributor

@jethrodaniel jethrodaniel left a comment

Choose a reason for hiding this comment

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

If you're adding development info, you might also want to add something about using bundler as well

git clone https://github.com/ruby-ldap/ruby-net-ldap
cd ruby-net-ldap
gem install bundler # if you don't have it
bundle # or bundle install
bundle exec rake rubotest

In fact, bundler usage is mentioned in CONTRIBUTING.md.

Additionally, gem build ..., is usually followed by gem install ... - or you can use bundler's rake tasks to do that for you (bundle exec rake build, bundle exec rake install).


Also, the changes to lib/net/ldap.rb would be best as another PR.

@dhivyada
Copy link
Author

Thank you for the feedback @jethrodaniel. Have updated the README as per your suggestions. Will raise separate PRs for the rest of the changes

@jethrodaniel
Copy link
Contributor

👍 thanks

:hosts => @hosts,
:encryption => @encryption,
:instrumentation_service => @instrumentation_service,
:socket_class => @socket_class,
Copy link
Member

Choose a reason for hiding this comment

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

Changes to this file should be in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

@dhivyada ☝️

:hosts => @hosts,
:encryption => @encryption,
:instrumentation_service => @instrumentation_service,
:socket_class => @socket_class,
Copy link
Member

Choose a reason for hiding this comment

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

@dhivyada ☝️

@HarlemSquirrel
Copy link
Member

Closing as stale. Please open a new PR if there' something else you'd like to add. Thank you!

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.

4 participants