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

Create Config class and fix printing. #456

Merged
merged 6 commits into from
Mar 25, 2022

Conversation

pvk-developer
Copy link
Member

@pvk-developer pvk-developer commented Mar 24, 2022

Resolve #450
Resolve #452
Resolve #449

  • Bump Faker version due to a bug with the deepcopy (version 9 and above fixes it).
  • Bumped Faker limit to 14 since they are currently on 13.

@pvk-developer pvk-developer requested a review from a team as a code owner March 24, 2022 23:57
@pvk-developer pvk-developer requested review from amontanez24 and removed request for a team March 24, 2022 23:57
@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2022

Codecov Report

Merging #456 (416fd02) into v1.0.0.dev (ce5dcf1) will not change coverage.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##           v1.0.0.dev      #456   +/-   ##
============================================
  Coverage      100.00%   100.00%           
============================================
  Files              14        14           
  Lines            1131      1135    +4     
============================================
+ Hits             1131      1135    +4     
Impacted Files Coverage Δ
rdt/hyper_transformer.py 100.00% <100.00%> (ø)
rdt/transformers/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce5dcf1...416fd02. Read the comment docs.

@pvk-developer pvk-developer force-pushed the issue_450_pretty_print_get_config branch from 90a8e55 to 12b5bfb Compare March 25, 2022 00:53
"""Config dict for ``HyperTransformer`` with a better representation."""

def __repr__(self):
"""Preaty print the dictionary."""
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: Pretty

@@ -12,6 +12,18 @@
get_default_transformer, get_transformer_instance, get_transformers_by_type)


class Config(dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have this class I'm wondering if it makes sense to move the field_transformers, _provided_field_transformers, field_sdtypes and _provided_field_sdtypes to this class. Then we could make the config an attribute of the HyperTransformer and add methods to this class for updating the config. For example, if the user sets something, it will update both dictionaries appropriately. Like a private update_field_transformers(self, user_provided=False) and update_field_sdtypes(self, user_provided=False) method that either only update the main dictionaries or both if necessary.

It's up to you if you want to do that extra work. I think the benefit would be that it would be less likely for anyone to forget to update or clear a provided dictionary when updating the main one, but this isn't in the scope of the issue

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened an issue to address this in another pr : #457

@pvk-developer pvk-developer merged commit 8504453 into v1.0.0.dev Mar 25, 2022
@pvk-developer pvk-developer deleted the issue_450_pretty_print_get_config branch March 25, 2022 21:15
amontanez24 pushed a commit that referenced this pull request Apr 21, 2022
* Create Config class and fix printing.

* Fix: integer and float to be numerical.

* Remove int and float.

* Fix integration test

* Fix tests

* Fix typo
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.

3 participants