-
Notifications
You must be signed in to change notification settings - Fork 7
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
Support property attributes in repr
of BaseMixin
#469
Conversation
🚀 Deployed on https://deploy-preview-469--etna-docs.netlify.app |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #469 +/- ##
==========================================
- Coverage 90.62% 90.60% -0.02%
==========================================
Files 247 247
Lines 16481 16481
==========================================
- Hits 14936 14933 -3
- Misses 1545 1548 +3 ☔ View full report in Codecov by Sentry. |
repr
of BaseMixin
CHANGELOG.md
Outdated
@@ -29,7 +29,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
- | |||
|
|||
### Changed | |||
- | |||
- Add support of property attributes in `__repr__` of `BaseMixin` ([#469](https://github.com/etna-team/etna/pull/469)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only __repr__
, but also to_dict
.
@@ -32,11 +32,11 @@ def __repr__(self): | |||
if param.kind == param.VAR_POSITIONAL: | |||
continue | |||
elif param.kind == param.VAR_KEYWORD: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create some basic tests on working with BaseMixin
. The methods to functionality to check is:
repr
- Add test on dummy class with private attribute and public property
to_dict
- We already have file
test_to_dict
, let's add new tests here - Add test on dummy class with private attribute and public property
- We already have file
set_params
- We already have file
test_set_params
, let's add new tests here - Add test on dummy class with private attribute and public property
- We already have file
tests/test_core/test_set_params.py
Outdated
@@ -130,3 +131,11 @@ def test_update_nested_structure(nested_structure, keys, value, expected_result) | |||
def test_update_nested_structure_fail(nested_structure, keys, value): | |||
with pytest.raises(ValueError, match=f"Structure to update is .* with type .*"): | |||
_ = BaseMixin._update_nested_structure(nested_structure, keys, value) | |||
|
|||
|
|||
def test_to_dict_public_property_private_attribute(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the test could be changed.
tests/test_core/test_set_params.py
Outdated
|
||
|
||
def test_to_dict_public_property_private_attribute(): | ||
dummy = Dummy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's may be set some values for a
and b
here for better inspectability.
tests/test_core/test_to_dict.py
Outdated
|
||
|
||
def test_to_dict_public_property_private_attribute(): | ||
dummy = Dummy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's may be set some values for a
and b
here for better inspectability.
tests/test_core/test_repr.py
Outdated
|
||
|
||
def test_repr_public_property_private_attribute(): | ||
dummy = Dummy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's may be set some values for a
and b
here for better inspectability.
tests/test_core/conftest.py
Outdated
from etna.core.mixins import BaseMixin | ||
|
||
|
||
class Dummy(BaseMixin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be we could name it somewhat more specific like: BaseDummy
to be related to BaseMixin
.
Before submitting (must do checklist)
Proposed Changes
Closing issues
closes #467