-
Notifications
You must be signed in to change notification settings - Fork 82
Format data_analyzer
and text_generation_plugins
#370
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
Conversation
data_analyzer
, text_generation_plugins
data_analyzer
and text_generation_plugins
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.
Good stuff, just some typos and maybe consistency issue with type checking!
dbldatagen/data_analyzer.py
Outdated
:param dfData: Source ``DataFrame`` to summarize | ||
:param rowLimit: Number of rows to use for ``DataFrame`` summarization | ||
:param dfSummary: Summary metrics ``DataFrame`` | ||
:return: Summary metrics ``DataFrame`` with the added measure |
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.
returns plural here
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.
Fixed ✅
dbldatagen/text_generator_plugins.py
Outdated
:param mname: Method name to invoke | ||
:param args: Positional argumentss to pass to the Faker text generation method | ||
:param _lib: Optional import alias of Faker library (dfault is ``"faker"``) |
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.
spelling "default"
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.
Fixed ✅
dbldatagen/data_analyzer.py
Outdated
result = f"""minValue={minValue}, maxValue={maxValue}, step=0.1""" | ||
elif sqlType == TimestampType(): | ||
|
||
elif isinstance(sqlType, types.DecimalType): |
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.
should we keep this as isinstance or replace with ==. If we want to have this take into account inheritance should we make the other typechecks also isinstance rather than ==?
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.
I noticed the input is the actual instance types.DecimalType()
, so we need to use equality and we'll need to keep the heuristic minimum and maximum values.
Changes
Applies formatting and adds type hints to the following modules:
data_analyzer.py
text_generation_plugins.py
Linked issues
N/A
Requirements