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

feat: implement from_params methods #18

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

Conversation

xXenvy
Copy link
Contributor

@xXenvy xXenvy commented Sep 7, 2024

No description provided.

@xXenvy xXenvy marked this pull request as draft September 7, 2024 21:34
@xXenvy xXenvy changed the title feat: implement from_params method feat: implement from_params methods Sep 8, 2024
@xXenvy xXenvy marked this pull request as ready for review September 8, 2024 05:45
from devana.syntax_abstraction.classinfo import *
import unittest

class TestInstanceCreations(unittest.TestCase):
Copy link
Owner

Choose a reason for hiding this comment

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

Please, add failure tests based on some locally created classes in the test body - how it behaves in case of errors. Additionally, it would be good to check in one test that propwerty becomes a property instead of being overwritten with the value of nstrwardo. I know that's how the code currently works, but if it was the first thing I thought of when reading the code as a possible error - it should get a test.

"""Checks if the attribute is directly in the instance or if it's a class attribute,
excluding properties."""
if hasattr(instance.__class__, name):
if not isinstance(getattr(instance.__class__, name, property()), property):
Copy link
Owner

Choose a reason for hiding this comment

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

You check if an attribute exists, adding a default one in this case is a bit pointless.

Comment on lines +14 to +16
try:
maybe_property = getattr(instance.__class__, name)
except AttributeError:
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe it's easier to check if the attribute exists? Unless you're protecting yourself from other errors here?

Comment on lines +7 to +8
"""A decorator that assigns method parameters as instance attributes.
The attribute must have a setter or exist as an instance or class variable."""
Copy link
Owner

Choose a reason for hiding this comment

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

Please document the use of this in a second paragraph and how it is supposed to work technically.

Comment on lines +45 to +46
) -> "CodeContainer":
return cls(None, parent)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a big fan of this notation. Ultimately it's something I can accept but if you feel like playing/testing you could think of a property on the class that would generate a method that would take all these parameters - then in this function we would call it explicitly.
But that's just thinking out loud.

cls,
parent: Optional = None,
content: Optional = None,
namespace: Optional = None,
Copy link
Owner

Choose a reason for hiding this comment

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

Why namespace is ignored?

@@ -124,6 +137,19 @@ def from_cursor(cls, cursor: cindex.Cursor, parent: Optional = None) -> Optional
result = cls(cursor, parent)
return result

@classmethod
@init_params(skip={"cls", "parent"})
Copy link
Owner

Choose a reason for hiding this comment

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

If we almost always ignore cls and parent maybe we can get rid of that parameter and add it to the implementation?

Comment on lines +46 to +49
cls,
parent: Optional = None,
namespaces: Optional = None,
lexicon: Optional = None
Copy link
Owner

Choose a reason for hiding this comment

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

Why isn't this typed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants