Skip to content

MemberSearch cleanup and polish #69

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
mtodd merged 7 commits into from
Dec 5, 2014
Merged

MemberSearch cleanup and polish #69

mtodd merged 7 commits into from
Dec 5, 2014

Conversation

@mtodd
Copy link
Member

@mtodd mtodd commented Dec 3, 2014

As a follow up to #64, this renames GitHub::Ldap::Members member search strategy to GitHub::Ldap::MemberSearch and aims to finish up any significant missing pieces:

  • configuration
  • Active Directory strategy (followup?)
  • detect strategy

cc @jch

jch
jch reviewed Dec 4, 2014
View reviewed changes
lib/github/ldap/member_search/detect.rb
Copy link
Contributor

@jch jch Dec 4, 2014

Choose a reason for hiding this comment

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

This module feels like a premature extraction. It's specific to ActiveDirectory, and doesn't seem useful outside this context.

Copy link
Member Author

@mtodd mtodd Dec 4, 2014

Choose a reason for hiding this comment

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

@jch agree in some senses, but there are two separate classes that depend on the same behavior that didn't feel right duplicating in (the ActiveDirectory MemberSearch and MembershipValidation strategies). It's a mix of AD-specific feature detection with general capability exposure.

I'm not crazy about it, but it unblocks this PR and can be easily changed in the future when a better approach can be considered, particularly because it's internal only. I think this need will diminish with the changes you outline in #69 (comment), so it is likely temporary.

Thoughts?

@mtodd
Copy link
Member Author

mtodd commented Dec 5, 2014

@jch so to summarize what needs to be addressed in a followup PR:

  • reduce detect_strategy's knowledge of strategy internals
  • give configuration more responsibility (setting the strategy object, defaulting)
  • move Active Directory capability sniffing functionality (either more central, or more context-specific)
  • possibly remove detect as a meta-strategy and incorporate into default configuration handling

Does that sound like a reasonable breakdown? Some of it may be more specific that necessary, partly because I think there are a few directions we could go that would result in cleaner internals with equivalent functionality, but exactly what I've not concluded.

@jch
Copy link
Contributor

jch commented Dec 5, 2014

👍 to shipping if you feel it'll unblock your other work. Left comments inline about refactorings.

mtodd added a commit that referenced this pull request Dec 5, 2014
@mtodd
Merge pull request #69 from github/member-search-cleanup
3a94c85 
MemberSearch cleanup and polish
@mtodd mtodd merged commit 3a94c85 into master Dec 5, 2014
@mtodd mtodd deleted the member-search-cleanup branch December 5, 2014 01:25
@mtodd mtodd mentioned this pull request Dec 5, 2014
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mtodd @jch