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

PlantUML generation by pyreverse may be incorrect #6683

Open
Alexander-Serov opened this issue May 24, 2022 · 6 comments
Open

PlantUML generation by pyreverse may be incorrect #6683

Alexander-Serov opened this issue May 24, 2022 · 6 comments
Labels
Bug 🪲 pyreverse Related to pyreverse component

Comments

@Alexander-Serov
Copy link

Alexander-Serov commented May 24, 2022

Bug description

Following PlantUML introduction in #4846, I have tried the following:

pyreverse -o plantuml <mypackage>

The resulting classes.plantuml file looks like this:

@startuml classes
set namespaceSeparator none
class "Command" as C:\Users\user\Documents\git\package\<...>\file1.py.Command {  ' <-- here
  name
}
class "Config" as C:\Users\user\Documents\git\package\<...>\config.py.Config {  ' <-- here
  filepath : Path
  ...
  to_dict()
}
...
@enduml

which is kind of correct, but the quotation mark and the as should be the other way, I would say:

@startuml classes
set namespaceSeparator none
class "C:\Users\user\Documents\git\package\<...>\file1.py.Command" as Command {  ' <-- here
  name
}
class "C:\Users\user\Documents\git\package\<...>\config.py.Config" as Config {  ' <-- here
  filepath : Path
  ...
  to_dict()
}
...
@enduml

The original version produced by pyreverse does not compile:
image

While it compiles after the fix above.

Command used

pyreverse -o plantuml <mypackage>

Pylint output

The parsing messages look correct, no errors:

parsing C:\Users\user\Documents\git\<...>\file.py...

Expected behavior

Qutation marks and as inversed: see bug description.

Pylint version

pylint 2.13.9
astroid 2.11.5
Python 3.8.12 | packaged by conda-forge | (default, Oct 12 2021, 21:19:05) [MSC v.1916 64 bit (AMD64)]

OS / Environment

Win11

@Alexander-Serov Alexander-Serov added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label May 24, 2022
@DanielNoord DanielNoord added Bug 🪲 pyreverse Related to pyreverse component and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 24, 2022
@Alexander-Serov Alexander-Serov changed the title PlantUML generation by pyreverse quatation may be incorrect PlantUML generation by pyreverse may be incorrect May 24, 2022
@DudeNr33
Copy link
Collaborator

Thank you for reporting this issue.
The quotation marks are correct the way they are now, but of course the output produced should be valid PlantUML.
You omitted some parts of the path in your example (<...>). Could it be that this part contains a space?

I'd also be interested in the structure of your package/directory, as the part after the as should normally be the qualified name, and not a path.

Some more explanation about the placement of the quotes:
No matter on what side of the as the quoted string is placed, it will always be the one that is used as the "label" for the class node in the final diagram.

Now, pyreverse allows to just display only the class name without the module names in front (this is the default behaviour and can be changed with -my).
But as you can have classes with the same name in different modules, we must use a unique name when defining the relationships between classes, because otherwise the arrows could be drawn between the wrong nodes. That's why pyreverse uses the format class "<NAME TO DISPLAY>" as UNIQUE_NAME.
To give a concrete example:

@startuml
class "Animal" as entities.Animal
class "Animal" as serializers.Animal

entities.Animal --> serializers.Animal
@enduml

But that just as a side note, what you observed is definitely not the intended behaviour.
I first want to understand why there is a path instead the qualified name I would expect.
Then we can see if it this itself is a bug or, if it is not, if simply replacing spaces with underscores would be a solution.

@Alexander-Serov
Copy link
Author

Hi @DudeNr33,

Thanks for your fast reply, very interesting! I have learned plenty of stuff from your explanations. There is still some mystery then why it didn't produce a valid PlantUML.

So, to answer your questions and give some details:

  • I have run pyreverse -o plantuml <package> as seen in pip list on a package that has been installed as pip install -e <package> (it's our private package). I did not run it on a folder. I wonder if that may have impacted the names shown as paths? It's however the only package version existing in this venv, so this should not be a question of version conflict.
  • I have concealed part of the package path for privacy, but I can assure you there are no spaces or weird characters in the path. Except, naturally, for the colon after the disk letter.

After writing the part above, I have launched pyreverse again and have actually realized that the problem is only caused by one line, the last line before @enduml, which looks like this:

c:\users\user\documents\git\<package>\file.py.CustomError --|> c:\users\user\documents\git\<package>\file.py.CustomError2

This is the only arrow I have in the file (because there is no other inheritance in the project), but it fails. If I remove the first colon in this line, it does compile, but the arrow is not shown because the name does not correspond to a class defined above.

Let me know if this helps you reproduce the problem? Will be happy to provide further information.

@DudeNr33
Copy link
Collaborator

Hi @Alexander-Serov,

I tried to reproduce the issue with a similar setup (running pyreverse -o plantuml pylint from outside the repository root, while having pylint itself installed with -e), but without luck. The part without quotation marks shows up as expected with the qualified name.
This was on a Windows 10 machine.

Do you have any other repo/package which is not installed via pip where you could check if you also get paths instead of qualified names when running pyreverse?

@Alexander-Serov
Copy link
Author

Hi @DudeNr33,

Thanks for your reply and sorry for the delay. It took me some time to run tests.

The weird thing is that I confirm your results. I was unable to produce the same quotation marks on the scipy or pylint public packages I tried. However, it is still 100% reproducible on the same package I posted above. And I have got the same behavior on our other private package. I wonder if it may be a factor, but both packages belong to the same namespace, so their names look like namespace.package and I install both through pip install -e . because I actively develop them.

By the way, while testing on a third project, I found out that I was unable to launch pyreverse on it due to an import error. The package name looks like x.y-z (import x.y-z when used). I have tried all combinations of dots, dashes, and quotation marks, but was unable to run pyreverse on it by name. The only way I was able to do it was to run pyreverse . in the package folder. I am not sure it's the main problem here, I can file a separate issue with details if you like, but I thought, maybe these issues are related since our projects share the same namespace and name structure.

Returning to the original problem, I am not sure how to investigate further. The issue is 100% reproducible on this package and our other one, but not on public packages. Perhaps, the namespace or the editable installation is a problem? Please let me know if you have any ideas.

@jacobtylerwalls
Copy link
Member

Maybe try installing the main branch of astroid, which has some improvements for namespace packages? At least something useful to rule out.

@DanielNoord
Copy link
Collaborator

@jacobtylerwalls Not sure if it is relevant here, but didn't we also see some differences between installing pip install -e . and pip install . for namespace packages?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 pyreverse Related to pyreverse component
Projects
None yet
Development

No branches or pull requests

4 participants