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

extract methods #11

Open
mskoenz opened this issue Mar 16, 2021 · 15 comments
Open

extract methods #11

mskoenz opened this issue Mar 16, 2021 · 15 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@mskoenz
Copy link

mskoenz commented Mar 16, 2021

First: awesome package, thx a lot ^^

Second: am I not finding the option, or is is currently not possible to extract the methods of classes?

Best regards
mskoenz

@lucsorel lucsorel added enhancement New feature or request help wanted Extra attention is needed labels Mar 17, 2021
@lucsorel
Copy link
Owner

Hi @mskoenz,

Thank you for your interest, I am glad if this library is of any help to you.

Unfortunately you are right: py2puml does not extract the methods of the parsed classes yet. I initially wrote it to document a folder of tangled dataclasses representing the domain model of an application that I started to work for. I needed to see composition relationships between the dataclasses.

But it makes totally sense to improve the library so that it exports methods too. Would you like to contribute?

@jonykalavera
Copy link
Contributor

I'd like to give this a try. will create a pull request. @lucsorel: any ideas you already have about this?

@jonykalavera
Copy link
Contributor

Some basic goal would be to cover this point in the plantuml docs https://plantuml.com/class-diagram#090967fbee930909

@jonykalavera
Copy link
Contributor

jonykalavera commented Apr 15, 2022

how should it look?

@startuml
class Example {
   some_attr : int
   some_method(some_param: int = 1) -> date
}
@enduml

@jonykalavera
Copy link
Contributor

Singature could be done as the first example here https://docs.python.org/3/library/inspect.html#introspecting-callables-with-the-signature-object

>>> from inspect import signature
>>> def foo(a, *, b:int, **kwargs):
...     pass

>>> sig = signature(foo)

>>> str(sig)  # LIKE THIS
'(a, *, b:int, **kwargs)'

>>> str(sig.parameters['b'])
'b:int'

>>> sig.parameters['b'].annotation
<class 'int'>

@jonykalavera
Copy link
Contributor

jonykalavera commented Apr 15, 2022

will wait for feedback to proceed. ✌️
py2puml parsing

@lucsorel
Copy link
Owner

lucsorel commented Apr 20, 2022

hi @jonykalavera
Thank you for proposing your help, which I appreciate a lot. I was on holidays but I am back. However, I am not available a lot these days (young dad + busy time for the vegetable garden). Please, don't take it personally and be patient if I am slow to answer back 🙏

A couple answers and thoughts:

  1. yes, signature inspection seems to me the way to go
  2. about the way to type signature parameters and the return type, I am not sure
    1. you suggested some_method(some_param: int = 1) -> date which seems pythonesque (with the arrow)
    2. but the UML convention seems to be date some_method(some_param: int = 1) and void method_returning_nothin(some_param: int = 1); but I would prefer leaving the return type empty rather than indicating void (unless the return type is set to -> None in the code), because using type annotations for the return type of methods is not a common practice. What do you think?
  3. about relationships: if a.some_method() returns an instance of class B, should we draw an UML relationship between classes A and B? If yes, should it be an association relationship? I would suggest not to add relationship at all when handling methods and leave them for the constructors, what do you think?
  4. we should distinguish instance/class methods from static methods. I would add {static} in front of static methods only, and leave class methods without UML decoration: class methods (receiving the class as their first parameter) do not exist in the UML syntax
  5. when I created this library, I was interested only in the class/instance attributes because they tell how the business domain is structured. I am ok with adding the methods since it is a wish of the community. However, I am wondering whether it is time to introduce some options to py2puml to let the people express what should be output (attributes, methods, etc.). It may be premature to decide what is optional (and what would be the default choice!), but I would like to know what you think about that, if you please

@jonykalavera
Copy link
Contributor

jonykalavera commented Apr 21, 2022

hi @jonykalavera Thank you for proposing your help, which I appreciate a lot. I was on holidays but I am back. However, I am not available a lot these days (young dad + busy time for the vegetable garden). Please, don't take it personally and be patient if I am slow to answer back 🙏

No problem. I assume you do this in your free time. :)

A couple answers and thoughts:

  1. yes, signature inspection seems to me the way to go
    👍

  2. about the way to type signature parameters and the return type, I am not sure

  3. you suggested some_method(some_param: int = 1) -> date which seems pythonesque (with the arrow)

yeah, I think it is ok because, when no explicit {method} is set, the distinguishing criteria is the presence of the parenthesis. and I like it sticks to the python typing syntax. Although for our purposes it's probably better to explicitly prefix them with {method} anyway.

  1. but the UML convention seems to be date some_method(some_param: int = 1) and void method_returning_nothin(some_param: int = 1);

AFAIK there are no hard rules about the typing order; at least according to the docs: https://plantuml.com/class-diagram#090967fbee930909

Note that the syntax is highly flexible about type/name order.

but I would prefer leaving the return type empty rather than indicating void (unless the return type is set to -> None in the code), because using type annotations for the return type of methods is not a common practice. What do you think?
I think we should reflect the pythonic way to show it since the documentation Well I think if it is there we should show it

I'm with you. In short: let's not use void, I like staying pythonic. let's leave type empty if not hinted, and show it if it is.
IMHO I think regardless of general practices the tool should just reflect what is there. for me, the value of this tool is that it can be a trusty mirror that lets me constantly reflect upon the architecture and general shape of my project at a high level. WDYT?

  1. about relationships: if a.some_method() returns an instance of class B, should we draw a UML relationship between classes A and B? If yes, should it be an association relationship? I would suggest not to add relationships at all when handling methods and leave them for the constructors, what do you think?

I agree. but maybe I am biased. in any case, I think we should leave it out of the scope of this PR.

  1. we should distinguish instance/class methods from static methods. I would add {static} in front of static methods only, and leave class methods without UML decoration: class methods (receiving the class as their first parameter) do not exist in the UML syntax

I agree. it should not be too hard to do. similar to how we dealt with abstract classes. also, I noticed that the signature shows the self and cls parameters too. which at first I thought to remove but then maybe they are useful to distinguish them. WDYT?

  1. when I created this library, I was interested only in the class/instance attributes because they tell how the business domain is structured. I am ok with adding the methods since it is a wish of the community. However, I am wondering whether it is time to introduce some options to py2puml to let the people express what should be output (attributes, methods, etc.). It may be premature to decide what is optional (and what would be the default choice!), but I would like to know what you think about that if you please

I totally agree. we should provide customization options. especially if the pr about functions gets merged. I want that one but i can imagine some might not. for now, if we want to keep the expected behavior, we can add one option to the command default false.

Will wait for your comments to proceed

@lucsorel lucsorel added the in progress Somebody is working on it label May 6, 2022
@lucsorel
Copy link
Owner

lucsorel commented May 6, 2022

sorry for the delay, the house is covid19 positive here. It seems to me that the PR lacks some unit tests (for the generation of the UML signature). And could you have a look at what needs to be done in the Contributions and version update section, please, when you have time?

@lucsorel lucsorel removed the in progress Somebody is working on it label Feb 15, 2023
@grayfox88
Copy link

Hi @lucsorel , I'd like to help here and write some unit tests. Do you know how to proceed when the implementation already exists in @jonykalavera 's forked repository?

@lucsorel
Copy link
Owner

hi @grayfox88, thank you for popping in and proposing your help 😃

The work started in PR #30 and what remains to do is listed in this comment: #30 (comment). The add-methods branch needs rebasing and merging, I modified some core functionalities (homogenized the way type annotations are processed, particularly).

There was also the issue that using inspection (stuffs like an_object.__dict__) is that inspect.getmembers(a_class, callable) also yields all the dunder methods associated to inherited methods from extending a native Python class (like extending a dictionary, for example). Filtering them on the presence of '__' around the method names would remove user-defined dunder methods (which is possible, although not recommended by the community).

I suspect that this feature should document only the methods declared in the class from the methods inherited from parent classes. Thus, I wonder whether we should keep this inspection approach or switch to an analysis of the abstract syntax tree 🤷

Adding unit tests that would check the output of py2puml on these cases (extending a dictionary, extending a custom parent class with user-defined methods, a class with user-defined dunder methods, etc.) would help understanding what we want (or accept) as PlantUML outputs. Do you feel like adding them without touching what @jonykalavera did?

@grayfox88
Copy link

New PR created #43

@stiebels
Copy link

stiebels commented Jul 28, 2023

Hi @lucsorel @grayfox88 ,
Would love to see #43 merged. Any help needed?

@lucsorel
Copy link
Owner

lucsorel commented Aug 5, 2023

hi all 😃

PR #51 needs to be reviewed and merged first because it also introduces github actions in the project, including formatting and linting hooks. I would like to enforce an homogeneity of code style in this project as feature requests and contributions are coming. Feel free to review it to accelerate the process 🙏

Then, I will rebase and contribute on this PR.

@stiebels
Copy link

stiebels commented Aug 23, 2023

Alright, @lucsorel! I've reviewed the entire PR (took a bit...) and left a handful of comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants