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

add methods support. #30

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jonykalavera
Copy link
Contributor

@jonykalavera jonykalavera commented Apr 15, 2022

❗ WIP #11

@@ -103,19 +129,20 @@ def inspect_class_type(
handle_inheritance_relation(class_type, class_type_fqn, root_module_name, domain_relations)

def inspect_dataclass_type(
class_type: Type[dataclass],
class_type: Type,
Copy link
Contributor Author

@jonykalavera jonykalavera Apr 15, 2022

Choose a reason for hiding this comment

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

dataclass is just a decorator function.

Copy link
Owner

Choose a reason for hiding this comment

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

👌

@@ -16,5 +16,6 @@ def inspect_namedtuple_type(
attributes=[
UmlAttribute(tuple_field, 'Any', False)
for tuple_field in namedtuple_type._fields
]
],
methods=[],
Copy link
Contributor Author

@jonykalavera jonykalavera Apr 15, 2022

Choose a reason for hiding this comment

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

technically namedtuples can have methods. should we support it?

from typing import NamedTuple


class Employee(NamedTuple):
    """Represents an employee."""
    name: str
    id: int = 3

    def foo(self) -> str:
        return f'<Employee {self.name}, id={self.id}>'

emp = Employee('Hello')
print(emp)
print(emp.foo())
print(emp._fields)

```

Copy link
Owner

Choose a reason for hiding this comment

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

if it is easy to distinguish added methods (like the Employee.foo() one) from native methods of named tuples (like _asdict), let's do it. Otherwise, let's wait for the need to come and the feature to be requested 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's see after I figure it out for regular classes and then we get back to this one to do it or split it.

def inspect_methods(
definition_methods, class_type,
):
no_dunder = lambda x: not (x[0].startswith('__') or x[0].endswith('__'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just excluding magic methods

Copy link
Owner

Choose a reason for hiding this comment

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

please, make no_dunder a proper function that must be unit-tested. And please, give more meaningful names to variables (x -> member, for instance)

Copy link
Contributor Author

@jonykalavera jonykalavera Apr 21, 2022

Choose a reason for hiding this comment

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

😰 ups. lol I was really hacking it together. sure thing. also, I tested this with some other projects and it totally shows more than what we want to see. need to look a lot further into determining declared methods only.

@@ -1,15 +1,14 @@

from inspect import isabstract
import inspect
Copy link
Owner

Choose a reason for hiding this comment

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

guidelines favor named imports instead of module imports: named imports gives meaning to what is happening in the module, module imports clutters the module's memory footprint

def inspect_methods(
definition_methods, class_type,
):
no_dunder = lambda x: not (x[0].startswith('__') or x[0].endswith('__'))
Copy link
Owner

Choose a reason for hiding this comment

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

please, make no_dunder a proper function that must be unit-tested. And please, give more meaningful names to variables (x -> member, for instance)

definition_methods, class_type,
):
no_dunder = lambda x: not (x[0].startswith('__') or x[0].endswith('__'))
methods = filter(no_dunder, inspect.getmembers(class_type, callable))
Copy link
Owner

Choose a reason for hiding this comment

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

from inspect import getmembers

no_dunder = lambda x: not (x[0].startswith('__') or x[0].endswith('__'))
methods = filter(no_dunder, inspect.getmembers(class_type, callable))
for name, method in methods:
signature = inspect.signature(method)
Copy link
Owner

Choose a reason for hiding this comment

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

from inspect import signature

@@ -103,19 +129,20 @@ def inspect_class_type(
handle_inheritance_relation(class_type, class_type_fqn, root_module_name, domain_relations)

def inspect_dataclass_type(
class_type: Type[dataclass],
class_type: Type,
Copy link
Owner

Choose a reason for hiding this comment

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

👌

@@ -16,5 +16,6 @@ def inspect_namedtuple_type(
attributes=[
UmlAttribute(tuple_field, 'Any', False)
for tuple_field in namedtuple_type._fields
]
],
methods=[],
Copy link
Owner

Choose a reason for hiding this comment

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

if it is easy to distinguish added methods (like the Employee.foo() one) from native methods of named tuples (like _asdict), let's do it. Otherwise, let's wait for the need to come and the feature to be requested 😃

fqn=class_type_fqn,
attributes=definition_attrs,
methods=definition_methods,
is_abstract=inspect.isabstract(class_type)
Copy link
Owner

Choose a reason for hiding this comment

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

from inspect import isabstract

@@ -1,15 +1,14 @@

from inspect import isabstract
import inspect
from typing import Type, List, Dict

from re import compile
from dataclasses import dataclass
Copy link
Owner

Choose a reason for hiding this comment

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

dataclass seems not necessary anymore after your fix, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check.

signature = inspect.signature(method)
uml_method = UmlMethod(
name=name,
signature=str(signature),
Copy link
Owner

Choose a reason for hiding this comment

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

it is a convenient way to get the whole signature, but:

  • it does not output the signature in the PlantUML expected syntax see comments 2. in extract methods #11 (comment)
  • the way str(signature) behave is likely to change and break the feature

However, I don't like how I implemented the the way attributes detected by inspection (dataclasses, class attributes) are typed: builtins.date would be displayed instead of just date for example. I did a better job when detecting instance attributes in constructors, the whole would need harmonization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is a convenient way to get the whole signature, but:

* it does not output the signature in the PlantUML expected syntax see comments `2.` in [extract methods #11 (comment)](https://github.com/lucsorel/py2puml/issues/11#issuecomment-1104471337)

it does not guarantee it for sure. I should just iterate the structure we can rely on and build it from ther

* the way `str(signature)` behave is likely to change and break the feature

It might. yeah

However, I don't like how I implemented the the way attributes detected by inspection (dataclasses, class attributes) are typed: builtins.date would be displayed instead of just date for example. I did a better job when detecting instance attributes in constructors, the whole would need harmonization.

I'll make sure to take a closer look there.

Copy link
Owner

Choose a reason for hiding this comment

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

for now, let's leave the signature format like that, it is not how UML users expect it, but it is readable to python users

@@ -83,7 +74,37 @@ def inspect_static_attributes(
uml_attr = UmlAttribute(attr_name, attr_type, static=True)
definition_attrs.append(uml_attr)

return definition_attrs

def inspect_methods(
Copy link
Owner

Choose a reason for hiding this comment

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

this function needs unit-testing, once we agree on the implementation details

@lucsorel
Copy link
Owner

thank you for the great job. We're nearly there 😃

Please also read my reply here: #11 (comment) and let me know what you think 🙏

1 similar comment
@lucsorel
Copy link
Owner

thank you for the great job. We're nearly there 😃

Please also read my reply here: #11 (comment) and let me know what you think 🙏

@jonykalavera
Copy link
Contributor Author

won't work on this today. will save it for the weekend.

@lucsorel
Copy link
Owner

hi @jonykalavera, how are you?

I would like to see your work being merged in master, surely so do you 😃

I listed a couple of things to be done:

Do you need some help? No rush anyway, I just wanted to show some support now that I found a little bit of free time

@IllustratedMan-code
Copy link

@lucsorel @jonykalavera I would love to help with this! Let me know if there is anything I can do.

@IllustratedMan-code
Copy link

IllustratedMan-code commented Dec 14, 2022

I would just like to add that I tested this on the add-method branch and received the following error (on a fairly complicated project):

ValueError: no signature found for builtin <method 'clear' of 'dict' objects>

Adding a try except to py2puml.py seemed to solve the issue for me, though it obviously isn't ideal:

    for _, name, is_pkg in walk_packages([domain_path], f'{domain_module}.'):
        if not is_pkg:
            try:
                domain_item_module: ModuleType = import_module(name)
                inspect_module(
                    domain_item_module,
                    domain_module,
                    domain_items_by_fqn,
                    domain_relations
                )
            except ValueError as e:
                pass

@lucsorel
Copy link
Owner

lucsorel commented Dec 14, 2022

I would love to help with this! Let me know if there is anything I can do.

Hi @IllustratedMan-code, thank you for volunteering to help this issue move forward 😃

What needs to be done is listed in this message: #30 (comment)

One of the main issue is tackling #30 (comment), where too many unwanted methods show up (I believe methods from inherited classes and Python magic methods we don't want to show up in the PlantUML diagram). Using inspection (which is done here) is easy but makes it hard to distinguish coded methods from inherited ones / magic methods; the alternative it to parse the Abstract Syntax Tree (as done to detect instance attributes in __init__ constructors), but it is harder to code.

The other issue is to add unit tests along with the feature.

@lucsorel
Copy link
Owner

I would just like to add that I tested this on the add-method branch and received the following error (on a fairly complicated project):

ValueError: no signature found for builtin <method 'clear' of 'dict' objects>

Adding a try except to py2puml.py seemed to solve the issue for me, though it obviously isn't ideal:

    for _, name, is_pkg in walk_packages([domain_path], f'{domain_module}.'):
        if not is_pkg:
            try:
                ...
            except ValueError as e:
                pass

Could you provide the stacktrace of the error, please? I believe the project has a user-defined class which extends a dictionary and that the branch's feature attempts to extract information from a native method which has no signature. Can you confirm that such a class exists in the project?

@IllustratedMan-code
Copy link

It does indeed! I'll provide a full trace soon

@lucsorel lucsorel added the waiting for feedback questions waiting to be answered label Feb 12, 2023
@lucsorel lucsorel mentioned this pull request Mar 29, 2023
@grayfox88 grayfox88 mentioned this pull request Apr 9, 2023
@lucsorel lucsorel added the enhancement New feature or request label Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request waiting for feedback questions waiting to be answered
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants