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" <mzidek(a)redhat.com>
To: "Development of the System Security Services Daemon"
<sssd-devel(a)lists.fedorahosted.org>
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
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel