Was talking to Lukas about the name and came up with this suggestion, so instead of "whitespaces_replacement_string" to "override_default_whitespace", thoughts?
Dan ----- Original Message ----- From: "Michal Židek" <[email protected]> To: "Development of the System Security Services Daemon" <[email protected]> Sent: Thursday, July 24, 2014 12:12:49 PM Subject: Re: [SSSD] [PATCH] NSS: replace with space with specified character in names. On 07/24/2014 05:53 PM, Michal Židek wrote: > On 07/24/2014 11:28 AM, Lukas Slebodnik wrote: >> ehlo, >> >> This patch add possibility to replace whitespace in user and group >> names with >> a specified string. With string "-", sssd will return the same result as >> winbind enabled option "winbind normalize names" >> >> Resolves: >> https://fedorahosted.org/sssd/ticket/1854 >> >> patch is attached. >> >> LS >> > > Hi Lukas, > > src/config/SSSDConfig/__init__.py.in > 76 'whitespaces_replacement_string': _('All white spaces will be > replaced in group name with this string'), > > Not just group name, but also user name. IMO something > like this would be better: > "All white spaces in group or user names will be replaced with > this string." Or something similar. > > Also please put changes in > src/config/SSSDConfig/__init__.py.in and man page changes into a > separate patch. We will than ping a native speaker (Stephen ?) > to check the wording when the coding part is accepted. > > The option name could be at least a little shorter, maybe: > replace_whitespace_str > or > replace_spaces_by > > But this is not a strong requirement, if you do not like the above > names and can't come up with something better, we can stick to the > current name. > > 35 static const char *replace_whitespaces(TALLOC_CTX *mem_ctx, > ... <snip> > 76 while (*ptr != '\0') { > 77 if (isspace(*ptr)){ > 78 new_name = talloc_asprintf_append(... > 79 } else{ > ^^^^^ > Missing space after else > 80 new_name = talloc_asprintf_append(... > 81 } > > The patch seem to work fine. But functions like > replace_whitespaces and the reverse version can be > easily unit-tested. It would be great if you could > add unit tests for these two. > > Michal Also when you call the new functions, you use this pattern 887 name = reverse_replace_withespaces(dctx, name, 888 889 if (name == NULL) { 890 DEBUG(SSSDBG_CRIT_FAILURE, 891 "reverse_replace_withespaces... 892 continue; 893 } Other functions in this part set ret and jump to done if something goes wrong. We could do the same here. Michal _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
