Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dummytools.py: F821 Undefined name "parent" #1451

Closed
buhtz opened this issue Jun 19, 2023 · 10 comments · Fixed by #1470 or #1486
Closed

dummytools.py: F821 Undefined name "parent" #1451

buhtz opened this issue Jun 19, 2023 · 10 comments · Fixed by #1470 or #1486
Assignees
Labels
Bug Discussion decision or consensus needed Documentation Low relevant, but not urgent
Milestone

Comments

@buhtz
Copy link
Member

buhtz commented Jun 19, 2023

While reviewing my own PR #1448 with linters (ruff & pycodestyle) I found a possible bug. Further investigation needed.

self.setattrKwargs('password', self.config.password(parent, self.profile_id), store = False, **kwargs)

Ruff says common/dummytools.py:51:61: F821 Undefined name "parent".

  • When was it introduced (commit history)?
  • What happens there?
  • Why isn't there a known issue about that problem?
  • Do we need that code if it is never reached?
@buhtz buhtz added the Bug label Jun 19, 2023
@buhtz buhtz changed the title common/dummytools.py:51:61: F821 Undefined name parent dummytools.py: "F821 Undefined name parent" Jun 19, 2023
@buhtz buhtz changed the title dummytools.py: "F821 Undefined name parent" dummytools.py: F821 Undefined name "parent" Jun 19, 2023
@buhtz
Copy link
Member Author

buhtz commented Jun 19, 2023

self.setattrKwargs('password', self.config.password(parent, self.profile_id), store = False, **kwargs)

I assume that mount.MountControl.parent is meant instead of parent.

The class Dummy isn't used anywhere. It seems just to be an example about how to derive from mount.MountControl. My first suggestion would be to move this into the (user or development) documentation somewhere.

@buhtz buhtz added this to the upcoming release (1.3.4) milestone Jun 19, 2023
@buhtz buhtz added Documentation Discussion decision or consensus needed Low relevant, but not urgent labels Jun 19, 2023
@Germar
Copy link
Member

Germar commented Jun 19, 2023

Have a look in sshtools.py:

parent (QWidget): parent widget for QDialogs or ``None`` if there
is no parent (handled by
inherited :py:class:`mount.MountControl`)

Dummy is a template for future classes.

@buhtz
Copy link
Member Author

buhtz commented Jun 19, 2023

Dear Germar,
thanks for the quick reply.

Have a look in sshtools.py:

Not sure if I interpret that correct. So you mean I'm right with my assumption that parent should be replaced with self.parent?

Dummy is a template for future classes.

I don't get that point. That code is never used.
If it is a "template" in the meaning that other developers should use that code as a start to create there own mount.MountControl based classes then I would say this should go into documentation (e.g. docstrings) and not into productive code.

@Germar
Copy link
Member

Germar commented Jun 19, 2023

Not sure if I interpret that correct. So you mean I'm right with my assumption that parent should be replaced with self.parent?

No, parent is a QWidget or None set in class Mount in mount.py

If it is a "template" in the meaning that other developers should use that code as a start to create there own mount.MountControl based classes then I would say this should go into documentation (e.g. docstrings) and not into productive code.

I don't care. I think it's easier to just copy dummy.py when someone starts a new mount.MountControl based classes. But you can move it into dev-doc if you like.

@buhtz
Copy link
Member Author

buhtz commented Jun 19, 2023

Not sure if I interpret that correct. So you mean I'm right with my assumption that parent should be replaced with self.parent?

No, parent is a QWidget or None set in class Mount in mount.py

Sorry, I'm confused and don't get it.
Then how to solve the problem "Undefined name 'parent'"?

@Germar
Copy link
Member

Germar commented Jun 19, 2023

parent is defined in mount.MountControl. Dummy inherit from Mount.MountControl, so it contains all args and functions, that mount.MountControl defined.

To solve the undefined name problem you need to tell the tool to resolve the inheritance of the class

@buhtz buhtz self-assigned this Jun 22, 2023
@buhtz
Copy link
Member Author

buhtz commented Jul 5, 2023

I assume there is a misunderstanding. But maybe I can't see the forest for the trees. 😅

To solve the undefined name problem you need to tell the tool to resolve the inheritance of the class

The tool is able to resolve inheritance.

My point (and the problem of Python and the linter) is that the name/object parent is unknown in Dummy.__init__(). It is not a variable in the methods names space. It is not an argument.

I do understand that conceptually it is inherited from class Mount which can be seen here:

self.parent = parent

Because of that that line

self.setattrKwargs('password', self.config.password(parent, self.profile_id), store = False, **kwargs)

IMHO should be modified into

self.setattrKwargs('password', self.config.password(self.parent, self.profile_id), store = False, **kwargs) 
                                                    ^^^^^

Am I wrong here somehow? I don't want to break anything.

buhtz added a commit that referenced this issue Jul 13, 2023
The code from dummytools.py is moved into the module docstring of "mount.py". As discussed in #1451 the purpose of this code was to be a template for developers deriving from "mount.MountControl".
@aryoda
Copy link
Contributor

aryoda commented Jul 22, 2023

IMHO should be modified into

self.setattrKwargs('password', self.config.password(self.parent, self.profile_id), store = False, **kwargs) 
                                                    ^^^^^

Am I wrong here somehow? I don't want to break anything.

@buhtz If think self is required here to access the attribute of the (parent) object.
Without self it would refer to local or global variable (which do not exist).

Since I wasn't sure myself I have used this example code to check this:

class Person:
  def __init__(self, fname, lname):
    self.firstname = fname
    self.lastname = lname
    self.parent = "a variable in the parent class"

  def printname(self):
    print(self.firstname, self.lastname)


class Student(Person):
  def __init__(self, fname, lname, year):
    super().__init__(fname, lname)
    self.graduationyear = year

    print(f"parent (self): {self.parent}")  # works
    # print(f"parent: {parent}")            # NameError: name 'parent' is not defined. Did you mean: 'print'?

  def welcome(self):
    print("Welcome", self.firstname, self.lastname, "to the class of", self.graduationyear)



x = Student("Mike", "Olsen", 2019)
x.welcome()

@aryoda
Copy link
Contributor

aryoda commented Jul 22, 2023

@buhtz You have copied the template code of class Dummy from dummypytools.py into a doc string in mount.py via the commit 1003d36 but the code for class Dummy is still contained in dummytools.py (even though you wrote "move" in your commit message):

class Dummy(mount.MountControl):

Was this intentional?

BTW: If "parent is a QWidget" in some cases (which I have not checked) we should refactor this with our GUI redesign since the backend logic (in common) should never see any GUI object from (e.g. use the observer pattern instead to ask a listener)

@buhtz
Copy link
Member Author

buhtz commented Jul 23, 2023

@buhtz You have copied the template code of class Dummy from dummypytools.py into a doc string in mount.py via the commit 1003d36 but the code for class Dummy is still contained in dummytools.py (even though you wrote "move" in your commit message):

Autsch. I'll fix that mistake.
Reopen because common/dummytools.py still exists but should be deleted.

@buhtz buhtz reopened this Jul 23, 2023
buhtz added a commit that referenced this issue Jul 28, 2023
Always use "backintime" as identifier for syslog entries.

Previously syslog (or a component behind it) silently refused the identifier because of a whitespace character in it. The name and ID of the snapshot profile is now part of the message string instead of the identifier. Even the GUI component do use "backintime" as identifier.

BREAKING CHANGE: This does modify the syntax of syslog entries.

Fix #1483
Close #1451
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Discussion decision or consensus needed Documentation Low relevant, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants