pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.32k stars 1.14k forks source link

PlantUML generation by `pyreverse` may be incorrect #6683

Open Alexander-Serov opened 2 years ago

Alexander-Serov commented 2 years ago

Bug description

Following PlantUML introduction in https://github.com/PyCQA/pylint/pull/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

DudeNr33 commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

DanielNoord commented 2 years ago

@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?