Re: [Patch for Review] iSCSI Name Validation using regex.

[Sujith Pandel]

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 
> >  #
> >
> > +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 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.

Thanks and Regards,
Sujith Pandel

[Patch for Review] iSCSI Name Validation using regex.

[Vratislav Podzimek]

> 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 
> > >  #
> > >
> > > +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!