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

pyreverse: add PlantUML output #4846

Merged
merged 13 commits into from Aug 14, 2021

Conversation

DudeNr33
Copy link
Collaborator

@DudeNr33 DudeNr33 commented Aug 14, 2021

  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
✨ New feature

Description

This MR adds output in PlantUML format to pyreverse.
I wanted to make sure that the feature from #4551 also works properly with PlantUML, so I added a new class to the test files.
This made the list of changed files a bit longer as intended.

Closes #4498

for arg, ann in annotations.items()
)

label += fr"{func.name}({args}){return_type}\l"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same logic is used by PlantUmlPrinter, so I extracted a method and put it inside the base Printer class.

@@ -209,8 +207,7 @@ def run(self, args):
diadefs = handler.get_diadefs(project, linker)
finally:
sys.path.pop(0)
printer_class = VCGPrinter if self.config.output_format == "vcg" else DotPrinter
writer.DiagramWriter(self.config, printer_class).write(diadefs)
writer.DiagramWriter(self.config).write(diadefs)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced a factory function to get the printer class for a given filetype. This supports the Open-Closed-Principle (only factory function needs to be updated if a new printer is added). Also, the DiagramWriter can now obtain the printer class itself.

f"{arg.name}: {ann}" if ann else f"{arg.name}"
for arg, ann in annotations.items()
]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is used by both DotPrinter and PlantUmlPrinter. We could probably add this feature for VCG as well if somebody needs it.

@coveralls
Copy link

coveralls commented Aug 14, 2021

Pull Request Test Coverage Report for Build 1131107870

  • 76 of 78 (97.44%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 92.713%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/pyreverse/plantuml_printer.py 46 47 97.87%
pylint/pyreverse/printer.py 13 14 92.86%
Totals Coverage Status
Change from base Build 1130450232: 0.03%
Covered Lines: 13385
Relevant Lines: 14437

💛 - Coveralls

self.my_int = optional_int

def do_it(self, new_int: int) -> int:
return self.my_int + new_int
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new class is for checking that #4551 works correctly with PlantUML output too.

)
assert expected == generated, f"{files}{diff}"
os.remove(generated_file)
_assert_files_are_equal(generated_file)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for each test case is basically the same, but the used fixtures differ. To avoid duplicated code I extracted a helper function for it.
The setup_xy are fixtures as they
a) shall perform cleanup code after the test case finishes
b) test scope is comparing the output file with expectation, not the process of generating the files itself

Is there a way to specify the fixture to use in pytest.mark.parametrize?

@@ -136,4 +140,3 @@ def test_vcg_files(generated_file):
line for line in unified_diff(expected.splitlines(), generated.splitlines())
)
assert expected == generated, f"{files}{diff}"
os.remove(generated_file)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup is performed in the fixture, this os.remove is not necessary.

@DudeNr33 DudeNr33 marked this pull request as ready for review August 14, 2021 18:03
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactor + new plantuml writter look very good ! Thank you for being so persistent, for separating this huge feature into multiple merge request and for all the explanations you gave. It's definitely going to be one of the major feature of 2.10.

pylint/pyreverse/printer_factory.py Outdated Show resolved Hide resolved
tests/pyreverse/test_printer_factory.py Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member

Maybe we could try covering the 3 lines not covered in the new plantuml writter with a diagram that would trigger them?

@DudeNr33
Copy link
Collaborator Author

Thanks for all your reviews, too! 😁
I added more tests (one of which actually caught a bug!).
But one line won't be covered until I have added the last part of the original MR: colored output.
Fontcolor was already used for exceptions (which I added to the test data), but body color is not used yet.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.10.0 milestone Aug 14, 2021
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component pyreverse Related to pyreverse component labels Aug 14, 2021
@Pierre-Sassoulas
Copy link
Member

one line won't be covered until I have added the last part of the original MR: colored output.

Understood, let's merge this then :)

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

Successfully merging this pull request may close these issues.

Permit to generate plantuml diagram with pyreverse
3 participants