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

React-Table transformer #170

Merged
merged 5 commits into from
Aug 7, 2018
Merged

React-Table transformer #170

merged 5 commits into from
Aug 7, 2018

Conversation

twheys
Copy link
Contributor

@twheys twheys commented Aug 6, 2018

Added a react-table transformer and changed pivot behavior of the Pandas and CSV transformers to pivot any dimension and to transpose the data frame.

These transformers accept two arguments: pivot: List<Dimension> and transpose: bool

With pivoted dimensions, the tables will render dimension values across columns instead of rows. When transpose is true, the columns and rows will be swapped.

…d CSV transformers to pivot any dimension and to transpose the data frame
@coveralls
Copy link

coveralls commented Aug 6, 2018

Coverage Status

Coverage increased (+0.4%) to 92.766% when pulling 79996ef on reacttable-transformer into a0ee1b3 on fireant1.0.

if 1 < pd.isnull(df).sum():
return df.fillna(NULL_VALUE, limit=1)
return df
if offset < pd.isnull(df).sum():
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to have a comment on this and/or a docstring for offset. Right now I'm not really sure I understood this logic.



class Widget:
def __init__(self, *items: Metric):
def __init__(self, *items: Union[Metric, Operation]):
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are using *arg then I believe the typing annotation should be List[Union[Metric, Operation]]. Same applies for all usages of *arg in this pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fireant/utils.py Outdated
@@ -13,6 +13,34 @@ def deep_get(d, keys, default=None):
d_level = d_level[key]
return d_level


def setdeepattr(d, key, value):
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming here and in the function below could be improved. Set deep attribute is not really meaningful. Docstrings are welcome as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add some docs. The naming is meant to extend setattr which is a built in func.

fireant/utils.py Outdated
@@ -23,6 +51,12 @@ def slice_first(item):
return item


a = 'a' \
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this always 'a'? What's the point of those ternary ifs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what this is. going to remove it

@staticmethod
def transform_data(data_frame, item_map, dimension_display_values):
"""
WRITEME
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you forgot to write a docstring for this and other occurrences of WRITEME?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@@ -33,7 +33,7 @@ def test_single_metric(self):
expected = single_metric_df.copy()[[fm('votes')]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any tests specifically for the react table transformer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course, in fireant/tests/slicer/widgets/test_reacttable.py

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, github didn't load the diff as the file was too large.

row = {}
for key, value in zip(data_frame.index.names, index):
if key is None:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Some inline comments in these complex functions would be useful, otherwise it is hard to follow.

@twheys twheys merged commit cc98225 into fireant1.0 Aug 7, 2018
@twheys twheys deleted the reacttable-transformer branch August 7, 2018 09:41
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.

None yet

4 participants