-
Notifications
You must be signed in to change notification settings - Fork 310
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
Align default transformers between SDV and RDT #1506
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #1506 +/- ##
=======================================
Coverage 96.28% 96.28%
=======================================
Files 49 49
Lines 3931 3932 +1
=======================================
+ Hits 3785 3786 +1
Misses 146 146
☔ View full report in Codecov by Sentry. |
@@ -90,6 +97,9 @@ def test___init__(self, update_transformer_mock, mock_rdt): | |||
assert data_processor._hyper_transformer == mock_rdt.HyperTransformer.return_value | |||
update_transformer_mock.assert_called_with(True, False) | |||
|
|||
mock_default_transformers.assert_called_once() |
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.
you should mock the return value and make sure the id default is properly added and that the text one is deleted
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.
Yes done in d84def8
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.
Couple small comments but LGTM! 🎉
assert 'text' not in data_processor._transformers_by_sdtype | ||
assert 'id' in data_processor._transformers_by_sdtype | ||
assert data_processor._transformers_by_sdtype == expected_default_transformers |
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 last assert is probably all you need
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.
Yes, done in 9a01726
if version: | ||
version = re.sub(r'>=?', '==', version) | ||
version = re.sub(r"""['",]""", '', version) | ||
requirement += version | ||
versions.append(requirement) | ||
|
||
elif (line.startswith('install_requires = [') or | ||
line.startswith('pomegranate_requires = [')): | ||
elif (line.startswith('install_requires = [')): |
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.
can you leave a comment in the description of the PR explaining why we made this change and the one on line 74? If we're going to merge this in this PR it would be good to understand why
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.
LGTM!
Resolve #1484
Made a change in tasks.py to include
.dev
version of libraries and made the minimum version GitHub action pass.