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

Export AddRequest Struct variables in add.go #60

Merged
merged 6 commits into from Jun 7, 2016

Conversation

nohupped
Copy link
Contributor

wrote get methods to struct AddRequest in add.go, as the struct variables are not global, and it would be easy if an add request wants to be compared with another one before populating it.

wrote get methods to struct AddRequest, as the struct variables are not global, and if I want to compare two add requests before populating it.
Changed get method names
Added a SetDN method to AddRequest. A usecase is, the result returned from AD is of uppercase realm for DN, and if a comparison needs to be done against an openldap result, it has to be converted to something similar in lowercase. This method makes it easy, as the SetDN sets the value to the pointer. Eg: ```i.SetDN(r.ReplaceAllStringFunc(input, func(m string) string {
			return strings.ToLower(m)
		}))```
@nohupped nohupped changed the title Added get methods to AddRequest struct Added get methods to AddRequest struct and a SetDN to modify the DN of an AddRequest. Apr 19, 2016
@liggitt
Copy link
Contributor

liggitt commented Apr 19, 2016

All the request structs except AddRequest and ModifyRequest export their fields... I don't really see anything special about those that should be different. I think I'd be ok with just exporting DN and Attributes in AddRequest

@nohupped
Copy link
Contributor Author

That's so kind of you, Thank you. I was wondering about modifying DN of the addrequest. One issue I faced was while comparing AD and LDAP add requests, where AD was returning realms like "CN=abc,OU=xyz,DC=example,DC=com" whereas LDAP was returning "cn=abc,ou=xyz,dc=example,dc=com" (uppercase/lowercase difference). If adding this feature is something redundant, which can be done using a ModifyRequest, I will follow that.

@liggitt
Copy link
Contributor

liggitt commented Apr 19, 2016

that seems more like a normalization issue, not specific to add requests

@nohupped
Copy link
Contributor Author

Gotcha, Thank you.

@nohupped
Copy link
Contributor Author

nohupped commented Apr 20, 2016

@liggitt Would you prefer to keep the get methods, or keep the struct fields exported like DN and Attributes, like in del.go? My opinion is it would be great if you prefer to keep it exported in the latter way, as users will get the freedom to write methods to modify the original struct with their own wrapper structs in their projects as

type AddRequest struct {
    *ldap.AddRequest
}

func (a *AddRequest) SetDN(dn string) {
    a.DN = dn
}```

@liggitt
Copy link
Contributor

liggitt commented Apr 20, 2016

yeah, I would just export the fields

@nohupped
Copy link
Contributor Author

Thank you

@nohupped nohupped changed the title Added get methods to AddRequest struct and a SetDN to modify the DN of an AddRequest. Export AddRequest Struct variables in add.go Apr 20, 2016
@vetinari
Copy link
Contributor

This is also in 9b058ac (part of #49)

I need the exported ones to render the add / mod requests as LDIF

@nohupped
Copy link
Contributor Author

nohupped commented Apr 21, 2016

I was wondering, if it would be fine to expose the Attribute struct's fields

type Attribute struct {
    attrType string
    attrVals []string
}

in add.go as well?

@liggitt
Copy link
Contributor

liggitt commented May 2, 2016

I think it's ok to expose those, though I'd like to rename them to match the LDAP names before exposing them, so Type and Vals

@nohupped nohupped closed this Jun 7, 2016
@nohupped nohupped reopened this Jun 7, 2016
@nohupped
Copy link
Contributor Author

nohupped commented Jun 7, 2016

Hi @liggitt I was wondering if this can be merged, as we are using a forked one for our project temporarily.

@liggitt liggitt merged commit f94af26 into go-ldap:master Jun 7, 2016
@liggitt
Copy link
Contributor

liggitt commented Jun 8, 2016

@nohupped, merged, sorry for the delay
@vetinari, fyi, also broke out the change to export fields in ModifyRequest structs into #67 and merged that as well

@nohupped
Copy link
Contributor Author

nohupped commented Jun 8, 2016

Thank you @liggitt

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

3 participants