Re: [Freeipa-devel] [PATCH] 78 Redo boolean value encoding
On Mon, 2012-05-07 at 18:49 +0200, Jan Cholasta wrote: On 7.5.2012 17:59, Martin Kosek wrote: On Mon, 2012-05-07 at 14:48 +0200, Jan Cholasta wrote: Hi, this patch changes the way boolean values are encoded to LDAP boolean syntax. The code for encoding boolean values is moved from the Parameter class to the Encoder class, where the rest of LDAP encoding takes place. The patch removes encoding code from the Parameter class altogether, as all LDAP encoding should be done in the Encoder class. Unit tests show no regressions and fixes for related tickets (https://fedorahosted.org/freeipa/ticket/2039 and https://fedorahosted.org/freeipa/ticket/2616) seem to be intact. Honza The patch looks ok, unit tests pass and I also did not find any regression. I have just one concern - with this patch, encoding is bound to native Python type and not to our Param classes. This means that all Params based on the same native Python type (lets say, str) will be encoded in the same way. Are you sure that nobody would want to implement a str-based param that has a custom LDAP encoding? Martin I don't think we want to do this, see http://www.redhat.com/archives/freeipa-devel/2012-April/msg00138.html (especially the ending). Honza Ok. We may revisit this idea when the the need for custom per-Param encoding is justified and really needed. ACK. Pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 78 Redo boolean value encoding
On Mon, 2012-05-07 at 14:48 +0200, Jan Cholasta wrote: Hi, this patch changes the way boolean values are encoded to LDAP boolean syntax. The code for encoding boolean values is moved from the Parameter class to the Encoder class, where the rest of LDAP encoding takes place. The patch removes encoding code from the Parameter class altogether, as all LDAP encoding should be done in the Encoder class. Unit tests show no regressions and fixes for related tickets (https://fedorahosted.org/freeipa/ticket/2039 and https://fedorahosted.org/freeipa/ticket/2616) seem to be intact. Honza The patch looks ok, unit tests pass and I also did not find any regression. I have just one concern - with this patch, encoding is bound to native Python type and not to our Param classes. This means that all Params based on the same native Python type (lets say, str) will be encoded in the same way. Are you sure that nobody would want to implement a str-based param that has a custom LDAP encoding? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 78 Redo boolean value encoding
On 7.5.2012 17:59, Martin Kosek wrote: On Mon, 2012-05-07 at 14:48 +0200, Jan Cholasta wrote: Hi, this patch changes the way boolean values are encoded to LDAP boolean syntax. The code for encoding boolean values is moved from the Parameter class to the Encoder class, where the rest of LDAP encoding takes place. The patch removes encoding code from the Parameter class altogether, as all LDAP encoding should be done in the Encoder class. Unit tests show no regressions and fixes for related tickets (https://fedorahosted.org/freeipa/ticket/2039 and https://fedorahosted.org/freeipa/ticket/2616) seem to be intact. Honza The patch looks ok, unit tests pass and I also did not find any regression. I have just one concern - with this patch, encoding is bound to native Python type and not to our Param classes. This means that all Params based on the same native Python type (lets say, str) will be encoded in the same way. Are you sure that nobody would want to implement a str-based param that has a custom LDAP encoding? Martin I don't think we want to do this, see http://www.redhat.com/archives/freeipa-devel/2012-April/msg00138.html (especially the ending). Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
