[master][PATCH] Use the right index for selecting region (#1114234)

[Vratislav Podzimek]

'id' is a Python builtin, 'index' is a variable that is used as an index in that
block of code.

Signed-off-by: Vratislav Podzimek 
---
 pyanaconda/ui/tui/spokes/time_spoke.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pyanaconda/ui/tui/spokes/time_spoke.py b/pyanaconda/ui/tui/spokes/time_spoke.py
index 62a1b23..3995a70 100644
--- a/pyanaconda/ui/tui/spokes/time_spoke.py
+++ b/pyanaconda/ui/tui/spokes/time_spoke.py
@@ -104,7 +104,7 @@ class TimeZoneSpoke(FirstbootSpokeMixIn, NormalTUISpoke):
                     self.apply()
                     self.close()
                 else:
-                    self.app.switch_screen(self, self._regions[id])
+                    self.app.switch_screen(self, self._regions[index])
                 return INPUT_PROCESSED
             elif key.lower() == "b":
                 self.app.switch_screen(self, None)

[Chris Lumens]

> diff --git a/pyanaconda/ui/tui/spokes/time_spoke.py b/pyanaconda/ui/tui/spokes/time_spoke.py
> index 62a1b23..3995a70 100644
> --- a/pyanaconda/ui/tui/spokes/time_spoke.py
> +++ b/pyanaconda/ui/tui/spokes/time_spoke.py
> @@ -104,7 +104,7 @@ class TimeZoneSpoke(FirstbootSpokeMixIn, NormalTUISpoke):
>                      self.apply()
>                      self.close()
>                  else:
>                self.app.switch_screen(self, self._regions[id])
>                self.app.switch_screen(self, self._regions[index])
>            return INPUT_PROCESSED
>        elif key.lower() == "b":
>            self.app.switch_screen(self, None)

This should have been caught by pylint, yeah?

- Chris

[Chris Lumens]

> > diff --git a/pyanaconda/ui/tui/spokes/time_spoke.py b/pyanaconda/ui/tui/spokes/time_spoke.py
> > index 62a1b23..3995a70 100644
> > --- a/pyanaconda/ui/tui/spokes/time_spoke.py
> > +++ b/pyanaconda/ui/tui/spokes/time_spoke.py
> > @@ -104,7 +104,7 @@ class TimeZoneSpoke(FirstbootSpokeMixIn, NormalTUISpoke):
> >                      self.apply()
> >                      self.close()
> >                  else:
> >                self.app.switch_screen(self, self._regions[id])
> >                self.app.switch_screen(self, self._regions[index])
> >            return INPUT_PROCESSED
> >        elif key.lower() == "b":
> >            self.app.switch_screen(self, None)
>
> This should have been caught by pylint, yeah?

After poking around at this a little bit... no, not really.  I can make
"id" a bad variable name which shows that we are using it in a couple
places, but doesn't do anything for this case.  I can make "id" a bad
builtin function to use but that doesn't catch this either.  So we would
either have to write something for this (which I'm not especially
inclined to do) or just keep an eye out.

- Chris

[David Shea]

On 06/30/2014 05:16 PM, Chris Lumens wrote:
> > > diff --git a/pyanaconda/ui/tui/spokes/time_spoke.py b/pyanaconda/ui/tui/spokes/time_spoke.py
> > > index 62a1b23..3995a70 100644
> > > --- a/pyanaconda/ui/tui/spokes/time_spoke.py
> > > +++ b/pyanaconda/ui/tui/spokes/time_spoke.py
> > > @@ -104,7 +104,7 @@ class TimeZoneSpoke(FirstbootSpokeMixIn, NormalTUISpoke):
> > >                       self.apply()
> > >                       self.close()
> > >                   else:
> > >                self.app.switch_screen(self, self._regions[id])
> > >                self.app.switch_screen(self, self._regions[index])
> > >             return INPUT_PROCESSED
> > >         elif key.lower() == "b":
> > >             self.app.switch_screen(self, None)
> >
> > This should have been caught by pylint, yeah?
>
> After poking around at this a little bit... no, not really.  I can make
> "id" a bad variable name which shows that we are using it in a couple
> places, but doesn't do anything for this case.  I can make "id" a bad
> builtin function to use but that doesn't catch this either.  So we would
> either have to write something for this (which I'm not especially
> inclined to do) or just keep an eye out.
> Chris
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

Do you think it'd be something worth raising upstream? pylint doesn't 
have checks on list index operations, so maybe it could use some. Here's 
a quick dirty version as a pylint plugin: http://fpaste.org/114472/16488614/

[Vratislav Podzimek]

On Mon, 2014-06-30 at 17:48 -0400, David Shea wrote:
> On 06/30/2014 05:16 PM, Chris Lumens wrote:
> > > > diff --git a/pyanaconda/ui/tui/spokes/time_spoke.py b/pyanaconda/ui/tui/spokes/time_spoke.py
> > > > index 62a1b23..3995a70 100644
> > > > --- a/pyanaconda/ui/tui/spokes/time_spoke.py
> > > > +++ b/pyanaconda/ui/tui/spokes/time_spoke.py
> > > > @@ -104,7 +104,7 @@ class TimeZoneSpoke(FirstbootSpokeMixIn, NormalTUISpoke):
> > > >                       self.apply()
> > > >                       self.close()
> > > >                   else:
> > > >                self.app.switch_screen(self, self._regions[id])
> > > >                self.app.switch_screen(self, self._regions[index])
> > > >             return INPUT_PROCESSED
> > > >         elif key.lower() == "b":
> > > >             self.app.switch_screen(self, None)
> > >
> > > This should have been caught by pylint, yeah?
> >
> > After poking around at this a little bit... no, not really.  I can make
> > "id" a bad variable name which shows that we are using it in a couple
> > places, but doesn't do anything for this case.  I can make "id" a bad
> > builtin function to use but that doesn't catch this either.  So we would
> > either have to write something for this (which I'm not especially
> > inclined to do) or just keep an eye out.
> > Chris
> >
> > anaconda-patches mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
> Do you think it'd be something worth raising upstream? pylint doesn't 
> have checks on list index operations, so maybe it could use some. Here's 
> a quick dirty version as a pylint plugin: http://fpaste.org/114472/16488614/

I think this should definitely be done in the upstream code.

[Anne Mulhern]

----- Original Message -----
> From: "Vratislav Podzimek" 
> To: "anaconda patch review" 
> Sent: Tuesday, July 1, 2014 1:52:55 AM
> Subject: Re: [master][PATCH] Use the right index for selecting region	(#1114234)
>
> On Mon, 2014-06-30 at 17:48 -0400, David Shea wrote:
> > On 06/30/2014 05:16 PM, Chris Lumens wrote:
> > > > > diff --git a/pyanaconda/ui/tui/spokes/time_spoke.py
> > > > > b/pyanaconda/ui/tui/spokes/time_spoke.py
> > > > > index 62a1b23..3995a70 100644
> > > > > --- a/pyanaconda/ui/tui/spokes/time_spoke.py
> > > > > +++ b/pyanaconda/ui/tui/spokes/time_spoke.py
> > > > > @@ -104,7 +104,7 @@ class TimeZoneSpoke(FirstbootSpokeMixIn,
> > > > > NormalTUISpoke):
> > > > >                       self.apply()
> > > > >                       self.close()
> > > > >                   else:
> > > > >                self.app.switch_screen(self, self._regions[id])
> > > > >                self.app.switch_screen(self, self._regions[index])
> > > > >             return INPUT_PROCESSED
> > > > >         elif key.lower() == "b":
> > > > >             self.app.switch_screen(self, None)
> > > >
> > > > This should have been caught by pylint, yeah?
> > >
> > > After poking around at this a little bit... no, not really.  I can make
> > > "id" a bad variable name which shows that we are using it in a couple
> > > places, but doesn't do anything for this case.  I can make "id" a bad
> > > builtin function to use but that doesn't catch this either.  So we would
> > > either have to write something for this (which I'm not especially
> > > inclined to do) or just keep an eye out.
> > > Chris
> > >
> > > anaconda-patches mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
> >
> > Do you think it'd be something worth raising upstream? pylint doesn't
> > have checks on list index operations, so maybe it could use some. Here's
> > a quick dirty version as a pylint plugin:
> > http://fpaste.org/114472/16488614/
>
> I think this should definitely be done in the upstream code.
>
> --
> Vratislav Podzimek
>
> Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

Agreed.

- mulhern

[David Shea]

On 06/30/2014 05:16 PM, Chris Lumens wrote:
> > > diff --git a/pyanaconda/ui/tui/spokes/time_spoke.py b/pyanaconda/ui/tui/spokes/time_spoke.py
> > > index 62a1b23..3995a70 100644
> > > --- a/pyanaconda/ui/tui/spokes/time_spoke.py
> > > +++ b/pyanaconda/ui/tui/spokes/time_spoke.py
> > > @@ -104,7 +104,7 @@ class TimeZoneSpoke(FirstbootSpokeMixIn, NormalTUISpoke):
> > >                       self.apply()
> > >                       self.close()
> > >                   else:
> > >                self.app.switch_screen(self, self._regions[id])
> > >                self.app.switch_screen(self, self._regions[index])
> > >             return INPUT_PROCESSED
> > >         elif key.lower() == "b":
> > >             self.app.switch_screen(self, None)
> >
> > This should have been caught by pylint, yeah?
>
> After poking around at this a little bit... no, not really.  I can make
> "id" a bad variable name which shows that we are using it in a couple
> places, but doesn't do anything for this case.  I can make "id" a bad
> builtin function to use but that doesn't catch this either.  So we would
> either have to write something for this (which I'm not especially
> inclined to do) or just keep an eye out.

Created an issue upstream: https://bitbucket.org/logilab/pylint/issue/269/

I didn't find any other bad list accesses.