Dne 24.7.2014 v 10:00 David Kupka napsal(a):
On 07/23/2014 05:07 PM, Jan Cholasta wrote:
Hi,
On 23.7.2014 15:46, David Kupka wrote:
https://fedorahosted.org/freeipa/ticket/4244
1) Use "isinstance(X, Y)" instead of "type(X) is Y".
Thanks for advice, will try to remember.
2) When is "type(not_before) is str" or "type(not_after) is str" true?
The values coming from command options or LDAP should always be
datetime, never str.
Actually, it is never true. I don't know why I thought that there is
such option.
3) There are some misindentations:
+ raise ValidationError(name='not_after',
+ error='is before not_before!')
+ raise ValidationError(name='not_after',
+ error='is before not_before!')
+ raise ValidationError(name='not_before',
+ error='is after not_after!')
4) We don't do exclamation marks in errors messages.
You re right, it's probably better not to shout at customer :)
5) Generally, when you want to validate command options, you should look
into "options", not "entry_attrs".
6) This is not right:
+ result = self.api.Command.otptoken_find(ipatokenuniqueid=
+ entry_attrs.get('ipatokenuniqueid', None))['result']
This is:
+ result = self.api.Command.otptoken_show(keys[-1])['result']
Both works, but Martin explained me why is otptoken_show better and how
it actually works.
Honza
Few more nitpicks:
1) You can make _check_interval a little bit shorter by removing a
redundant if statement:
+def _check_interval(not_before, not_after):
+ if not_before and not_after:
+ return not_before <= not_after
+ return True
2) Please keep the 2 line space between the last global function and
first class in the module.
3) Error messages are for users' eyes, please don't use internal
identifiers in them.
4) You don't need to check if the result of otptoken_show is empty, it
never is.
--
Jan Cholasta
_______________________________________________
Freeipa-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/freeipa-devel
