On Wed, 2015-04-29 at 00:19 -0400, Sujith Pandel wrote:
Hi Vratislav,
Thank you for your feedback on this.
One of the changes which you mentioned in this:
> > --- a/pyanaconda/ui/gui/spokes/advstorage/iscsi.py
> > +++ b/pyanaconda/ui/gui/spokes/advstorage/iscsi.py
> > @@ -18,7 +18,7 @@
> > #
> > # Red Hat Author(s): Chris Lumens <clumens(a)redhat.com>
> > #
> > -
> > +import re
> > +from pyanaconda.regexes import ISCSI_IQN_NAME_REGEX, ISCSI_EUI_NAME_REGEX
> >....
> > + #iSCSI Name validation using regex. Name should either match IQN
format or EUI format.
> > + if re.match(ISCSI_IQN_NAME_REGEX,stripped) or
re.match(ISCSI_EUI_NAME_REGEX,stripped):
> We prefer calling the match() method of the compiled regexes instead of
> using re.match() on them. That would also remove allow you to get rid of
> the commas that would otherwise need a space right after them. :)
>
> > + return True
> > + else:
> > + return False
> This if statement should be reduced to just:
>
> return ISCSI_ION_NAME_REGEX.match(stripped) or ISCSI_EUI_NAME_REGEX.match(stripped)
>
>
Please note that this results in a run time exception when return value becomes
'None'. Verified this condition.
(Boolean values are expected at the receiving end.)
Based on your suggestions above, I am thinking of changing it to:
if ISCSI_IQN_NAME_REGEX.match(stripped) or ISCSI_EUI_NAME_REGEX.match(stripped):
return True
else:
return False
This could/should be done as:
return bool(regex1.match() or regex2.match())
This will also prevent the need to "import re" in iscsi.py
Let me know if any changes are required to it.
> > +import unittest
> > +import re
> Seems like the 're' module is not needed anywhere below.
>
Yes. I'll remove it.
I'll send the modified patch incorporating the changes once you confirm on these
changes.
Great, thanks!
--
Vratislav Podzimek
Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic