Add methods #43

grayfox88 · 2023-04-09T12:27:07Z

This pull request is based on old PR "#30 add methods support" created by jonykalavera

grayfox88 · 2023-04-09T12:36:32Z

This branch has not been rebased yet on master, as the initial goal is to create simple test cases to understand how methods should be represented in UML. My initial suggestion would be to sort methods by type as follow:

instance "special" methods defined by user (aka dunder method)
all other instance methods
static methods

@lucsorel, what's your view on method representation? Should they be sorted by type or appear in the same order as in the code? Any other test cases that shall be added before looking into the AST approach?
I agree that only method declared in the class shall be represented, not the one inherited.

lucsorel · 2023-04-11T21:09:12Z

tests/data/with_methods.puml

+  y: str
+  void __init__(x: int, y: str, unit: str, u: float, z: List[int])
+  (float, float) get_coordinates()
+  {static} Point from_values(x: int, y: str, u: float, z: List[int])


different remarks about the type of the returned value:

with @jonykalavera, we thought of:

annotating the type of the returned value "the Python way" (like from_values(x: int, y: str, u: float, z: List[int]) -> Point) because it would be pythonistas who are going to use the tool, not UML experts

not annotating anything if nothing is returned, instead of returning "void", because type annotations are optional in Python and can be used in an heterogeneous way across files. We thought that it would make more sense of specifying the type of the returned value only when it is specified

the type of the value by returned by Point.get_coordinates() could be more consistent with the way we would write it in Python: tuple, Tuple[float, float] or tuple[float, float], for instance (that would be what we actually chose in the parsed source code)

Since py2puml produces UML class diagrams I'd rather stick to the original UML syntax with return type declared before the signature. But I kept the Python return type hinting syntax in the next commit to leave this discussion opened.

I kind of agree with you, let's follow the UML convention. Using the Python type annotation syntax here was an artefact of using signature=str(signature) (see https://github.com/lucsorel/py2puml/pull/43/files#diff-adfc82ab9429d3c5fa93391a8044c10a783c3c77df333a23f8a5eaf31168f91fR87), but I didn't like relying on the str representation of a signature, which can change over time 👍

I reverted the expected results in the test cases to the UML convention in my latest commit .

lucsorel · 2023-04-11T21:23:27Z

First of all, thank you @grayfox88 for contributing to this issue 🙏 😃

what's your view on method representation?

I have answered partly to that on comment #43 (comment). I am not strongly opinionated about that and am looking for a consensus rather than giving directions. What do you think of sticking rather to Python type conventions rather than UML conventions?

Should they be sorted by type or appear in the same order as in the code?

In a pragmatic approach to reduce the workload involved in this feature request, I would say that the order should be what the parsing returns 😃 (which is likely to be the source code order). Depending on later feedback, we could consider sorting things.

Any other test cases that shall be added before looking into the AST approach?

I would suggest adding a test case with 2 classes, one extending the other, both of them having methods. So that we can make sure that inherited methods do not appear in the child class. What do you think?

lucsorel · 2023-04-11T21:25:46Z

I can help rebasing on the main branch; because it would take less time to apply the changes for me than for anyone else, I believe.

lucsorel · 2023-04-11T21:27:28Z

tests/py2puml/test_py2puml.py

+        expected_data = fref.read()
+    puml_content = py2puml.py2puml('tests/modules/withmethods', 'tests.modules.withmethods')
+    actual_data = ''.join(puml_content)
+    assert actual_data == expected_data


in the main branch, there are now functions dedicated to comparing the output of a parsing command with the expected PlantUML contents. We should use them after rebasing.

lucsorel · 2023-04-11T21:29:28Z

tests/py2puml/inspection/test_inspectclass.py

+    This test case assumes that the methods will be sorted by type as follow:
+        1a - instance methods (special methods aka "dunder")
+        1b - all other instance methods
+        2 - static methods """


in between, there can be class methods too

but as I said elsewhere, we can consider sorting methods later

Class methods are a bit tricky, because they are specific to Python (and maybe a dozen other languages...) and do not have a formal representation in the UML-language as such. One option could be to make use of "separators" to group class methods, but it would require to start sorting methods 😜 See code snippet below:

__Class methods__ first_class_method() second_class_method()

Using "icons" to represent a class method is not an option since these are reserved for other use.

Any reflection on that matter?

the 2 links (for separators and icons) seem broken: they don't seem to refer to the things you said.

However:

I see what separators are and they could be used

I believe the "icons" you mentioned are the one dedicated to visibility (private / public); thus I agree that we shouldn't use them for something else

As a first step, I suggest that we don't sort methods for now: this PR is already going to be big. What do you think?

Second question: should we keep the self and cls used in the methods' signature? I think we should, as they carry the characteristics of being either an instance or a class method. Static methods would have neither of them; thus we would not need to find visual things to distinguish them.

Agree both on both questions.
However for the static methods, I still think that we should stick to UML and use the {static} modifier.

ok: let's use the {static} modifier (as py2uml already uses it for class's attributes), let's sort later

grayfox88 · 2023-04-12T19:23:59Z

New test case has been added with subclass inheriting methods. I kept the Python-like typing syntax in this commit in the output puml-files but would prefer to adopt the official UML syntax. It is the formal way to describe class diagram and is meant to be a standard way of representing object oriented systems ... no matter which programming language is used!

The "U" in UML stands for "unified" after all 😉

tests/modules/withmethods/withmethods.py

lucsorel · 2023-04-12T20:42:10Z

tests/py2puml/inspection/test_inspectclass.py

+
+    assert point_umlitem.methods[0].name == 'from_values'
+    assert point_umlitem.methods[1].name == 'get_coordinates'
+    assert point_umlitem.methods[2].name == '__init__'


we should also assert the return types of the methods

Yes, but the return type is not available yet in the dataclass UmlMethod. I suggest to update the test case once this class has been fleshed out.

agreed 👍

tests/modules/withmethods/withmethods.py

tests/data/with_methods.puml

py2puml/domain/umlclass.py

lucsorel · 2023-04-12T21:05:39Z

New test case has been added with subclass inheriting methods. I kept the Python-like typing syntax in this commit in the output puml-files but would prefer to adopt the official UML syntax. It is the formal way to describe class diagram and is meant to be a standard way of representing object oriented systems ... no matter which programming language is used!
The "U" in UML stands for "unified" after all wink

I agree with you, see #43 (comment) 👍

grayfox88 · 2023-04-17T19:29:48Z

Hello @lucsorel, feel free to rebase this branch on top of main when you have time 😃 Or let me know if you want to correct some of the changes made to the test case.

lucsorel · 2023-04-20T00:41:27Z

I have rebased this branch on top of main. This required to force-push the new commits history. You may need to delete your local add-methods branch and checkout this new one.

And now, the implementation can start 😃

grayfox88 · 2023-05-21T16:35:08Z

@lucsorel, here comes a first fraft that implements methods using AST. It took me quite a while to get my head around the structure of this package and how the AST works but I do believe AST is the right way to go. Note that this is only a first implementation, the return type of the method is for example not included. I also noticed that I added to the astvisitors.py module a lot of unused import statement that will have to be removed in the next commit. Just wanted to get your input on this first attempt though ...
In a more general manner, I do have suggestion that could improve the cohesion of certain modules and improve the readability too. But we can discuss these refactoring suggestions later.

lucsorel · 2023-05-22T21:21:25Z

Thank you @grayfox88 for proposing a first draft of the implementation ❤️

I have not had a look at it yet and I am really busy with the summer holiday deadlines. Please, give me some time and don't be offended 🙏

grayfox88 · 2023-05-24T19:57:03Z

@lucsorel No worries. In the latest commit, I corrected some of the clumsy issues I mentioned in my earlier post (useless import statements, failing test cases) and attempted to implement the return_type representation. I'm still a bit puzzled when it comes to how AST visiting works so you might want to look into my code and suggests a better implementation since there is for example no proper support for compound type in the return type.

Also, more of a general question related to return type: if no type hinting is available in the source code, are we supposed to infer it? Or should we just omit it for the puml representation as done today?

grayfox88 · 2023-06-01T17:59:41Z

@lucsorel, I noticed that one of the test cases is failing: test_py2puml_with_methods.

The UML representation of the class ThreeDimensionalPoint contains wrongly the class variable PI that belongs to its parent class Point. The culprit seems to be the function inspectclass.inspect_static_attributes which relies on the dunder attribute annotations to retrieve class variables. See explanation from annotations official documentation:

The problem is that, since annotations is optional on classes, and because classes can inherit attributes from their base classes, accessing the annotations attribute of a class may inadvertently return the annotations dict of a base class.

AST would do some good here too 😉

Expected UML content:

class tests.modules.withmethods.withinheritedmethods.ThreeDimensionalPoint {
  z: float
  __init__(self, int x, str y, float z)
  move(self, int offset)
  bool check_positive(self)
}

Actual UML content:

class tests.modules.withmethods.withinheritedmethods.ThreeDimensionalPoint {
  PI: float {static}
  z: float
  __init__(self, int x, str y, float z)
  move(self, int offset)
  bool check_positive(self)
}

lucsorel · 2023-06-05T15:24:17Z

AST would do some good here too

Definitely! I started this project using inspection / reflexion because it was handy and easy (writing an AST parser is harder). But I now realize that I should give up the reflexion way because it cannot help separating inherited properties from the other ones.

Removing reflexion from the library will be a huge step, but it will make the library failproof to parsing artefacts. Particularly when an execution part is not protected in a if __name__ == '__main__': condition

grayfox88 · 2023-06-06T16:03:18Z

I agree that refactoring the whole code to embrace AST is a bigger nut to crack. Should this be handled in a separate pull-request?

Will soon push a new commit that solves this bug without having to go over to AST right now.

stiebels · 2023-09-08T23:11:02Z

py2puml/asserts.py

@@ -20,6 +20,7 @@ def assert_py2puml_is_stringio(domain_path: str, domain_module: str, expected_co
 def assert_multilines(actual_multilines: List[str], expected_multilines: Iterable[str]):
    line_index = 0
    for line_index, (actual_line, expected_line) in enumerate(zip(actual_multilines, expected_multilines)):
+        # print(actual_line[:-1])


Superfluous print statement. Can be removed?

lucsorel · 2023-10-16T07:53:56Z

hello @grayfox88 and @stiebels,

Forgive me for the long wait, I am ready to contribute to and review this branch. To ease the rebasing on main (and solving the conflicts), I would like to squash the commits of this branch; is that ok for you?

You will need to delete your local branch and check it out again, afterwards. Forgive me again for this inconvenience.

Have a nice day!

grayfox88 · 2023-10-23T10:19:39Z

Hello @lucsorel, yes go ahead and squash it.

lucsorel · 2023-11-22T08:40:06Z

hello @grayfox88 and @stiebels, I hope you are all right 😃

Forgive me for the long wait. Yesterday night, I squashed the commits of the branch and applied the automatic formatting on the code in the perspective of rebasing the branch on main.

In the mean time, the default version of python used to develop py2puml has moved on from 3.8 to 3.10 (to support the int | float syntax for union types). That move broke the implementation of TypeVisitor which receives instances of ast.Tuple (for example) instead of ast.Index ones when handling types involving brackets (see the deprecation notes of https://docs.python.org/3/library/ast.html#ast.AST, https://greentreesnakes.readthedocs.io/en/latest/nodes.html#Index and https://greentreesnakes.readthedocs.io/en/latest/nodes.html#Tuple). This changes the way inner elements (component types, in this case) can be reached. I'll try to fix that, or maybe get rid of the whole visitor since the project already has the shorten_compound_type_annotation function to standardize the type representation of variables, which can be used for argument types and return types of method as well.

Also, please do not introduce unittest in this project, please 🙏 ; for the sake of consistency with the rest of the codebase: unittest tests are more verbose than the pytest approach with isolated test functions. 😃

grayfox88 mentioned this pull request Apr 9, 2023

extract methods #11

Open

lucsorel reviewed Apr 11, 2023

View reviewed changes

lucsorel reviewed Apr 12, 2023

View reviewed changes

tests/modules/withmethods/withmethods.py Outdated Show resolved Hide resolved

lucsorel reviewed Apr 12, 2023

View reviewed changes

tests/modules/withmethods/withmethods.py Outdated Show resolved Hide resolved

lucsorel reviewed Apr 12, 2023

View reviewed changes

tests/data/with_methods.puml Outdated Show resolved Hide resolved

lucsorel reviewed Apr 12, 2023

View reviewed changes

py2puml/domain/umlclass.py Outdated Show resolved Hide resolved

lucsorel force-pushed the add-methods branch from ab299ea to 0974bf1 Compare April 20, 2023 00:38

lucsorel mentioned this pull request Aug 30, 2023

Add support for SQLAlchemy models #56

Open

stiebels reviewed Sep 8, 2023

View reviewed changes

lucsorel added the enhancement New feature or request label Oct 16, 2023

lucsorel force-pushed the add-methods branch from 29dea7a to a2750b2 Compare November 21, 2023 22:21

feat: add support to detect and document methods

ccf983f

lucsorel force-pushed the add-methods branch from a2750b2 to ccf983f Compare November 21, 2023 22:23

vignesh14052002 mentioned this pull request Jul 27, 2024

Expose controls on which block, method, relation can be included in uml diagram #108

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add methods #43

Add methods #43

grayfox88 commented Apr 9, 2023 •

edited

Loading

grayfox88 commented Apr 9, 2023

lucsorel Apr 11, 2023

grayfox88 Apr 12, 2023

lucsorel Apr 12, 2023

grayfox88 Apr 14, 2023

lucsorel commented Apr 11, 2023

lucsorel commented Apr 11, 2023

lucsorel Apr 11, 2023

grayfox88 Apr 12, 2023

lucsorel Apr 11, 2023

lucsorel Apr 11, 2023

grayfox88 Apr 12, 2023 •

edited

Loading

lucsorel Apr 12, 2023 •

edited

Loading

grayfox88 Apr 13, 2023

lucsorel Apr 14, 2023

grayfox88 commented Apr 12, 2023 •

edited

Loading

lucsorel Apr 12, 2023

grayfox88 Apr 13, 2023

lucsorel Apr 14, 2023

lucsorel commented Apr 12, 2023

grayfox88 commented Apr 17, 2023

lucsorel commented Apr 20, 2023

grayfox88 commented May 21, 2023

lucsorel commented May 22, 2023

grayfox88 commented May 24, 2023

grayfox88 commented Jun 1, 2023 •

edited

Loading

lucsorel commented Jun 5, 2023

grayfox88 commented Jun 6, 2023 •

edited

Loading

stiebels Sep 8, 2023

lucsorel commented Oct 16, 2023

grayfox88 commented Oct 23, 2023

lucsorel commented Nov 22, 2023

Add methods #43

Are you sure you want to change the base?

Add methods #43

Conversation

grayfox88 commented Apr 9, 2023 • edited Loading

grayfox88 commented Apr 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucsorel commented Apr 11, 2023

lucsorel commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grayfox88 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

lucsorel Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grayfox88 commented Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucsorel commented Apr 12, 2023

grayfox88 commented Apr 17, 2023

lucsorel commented Apr 20, 2023

grayfox88 commented May 21, 2023

lucsorel commented May 22, 2023

grayfox88 commented May 24, 2023

grayfox88 commented Jun 1, 2023 • edited Loading

lucsorel commented Jun 5, 2023

grayfox88 commented Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

lucsorel commented Oct 16, 2023

grayfox88 commented Oct 23, 2023

lucsorel commented Nov 22, 2023

grayfox88 commented Apr 9, 2023 •

edited

Loading

grayfox88 Apr 12, 2023 •

edited

Loading

lucsorel Apr 12, 2023 •

edited

Loading

grayfox88 commented Apr 12, 2023 •

edited

Loading

grayfox88 commented Jun 1, 2023 •

edited

Loading

grayfox88 commented Jun 6, 2023 •

edited

Loading