Grid Accept Dataframe #675
Conversation
|
@theengineear Mind taking a look when you have a minute? |
| from plotly import exceptions, utils | ||
| from plotly import exceptions, optional_imports, utils | ||
|
|
||
| pd = optional_imports.get_module('pandas') |
| @@ -148,7 +150,21 @@ def __init__(self, columns_or_json, fid=None): | |||
| ``` | |||
| """ | |||
| # TODO: verify that columns are actually columns | |||
| if isinstance(columns_or_json, dict): | |||
| if pd and isinstance(columns_or_json, pd.DataFrame): | |||
| column_names = [name for name in columns_or_json.columns] | |||
There was a problem hiding this comment.
🐄 if you need this to be a list, why not just list(columns_or_json.columns)? If you don't need this to be a list, why not just leave it as a pandas array? column_names = columns_or_json.columns?
| if isinstance(columns_or_json, dict): | ||
| if pd and isinstance(columns_or_json, pd.DataFrame): | ||
| column_names = [name for name in columns_or_json.columns] | ||
| duplicate_name = utils.get_first_duplicate(column_names) |
There was a problem hiding this comment.
ah, here we go, can you not just do utils.get_first_duplicate(columns_or_json.columns)?
|
|
||
| # create columns from dataframe | ||
| all_columns = [] | ||
| for name in column_names: |
There was a problem hiding this comment.
Same here, embrace the 🦆. for name in columns_or_json.columns:.
There was a problem hiding this comment.
Don't know why I'm afraid of Ducks...
| all_columns = [] | ||
| for name in column_names: | ||
| all_columns.append(Column(columns_or_json[name].tolist(), name)) | ||
| self._columns = list(all_columns) |
There was a problem hiding this comment.
Why list this? You've already instantiated a new list above all_columns = []. I don't see why an additional shallow copy is necessary?
| import pandas as pd | ||
|
|
||
|
|
||
| class Test_Dataframe_to_Grid(TestCase): |
There was a problem hiding this comment.
pep:8ball: haha class names should be like TestDataframeToGrid or something. This is like 🐍 + 🐫 or something.
There was a problem hiding this comment.
Two things:
- What does 🐍 mean?
- Why would DRY be relevant here? What am I repeating?
I'm changing it to your suggested class name, just curious
There was a problem hiding this comment.
Sorry, I can see why that's confusing ;__;.
I was talking about camelCase vs snake_case (hence the 🐍 and 🐫).
| # Test duplicate columns | ||
| def test_duplicate_columns(self): | ||
| df = pd.DataFrame([[1, 'a'], | ||
| [2, 'b']], columns=['col_1', 'col_1']) |
There was a problem hiding this comment.
🐄 this is a weird way to break this line, I'd suggest:
df = pd.DataFrame([[1, 'a'], [2, 'b']],
columns=['col_1', 'col_1'])
| df = pd.DataFrame([[1, 'a'], | ||
| [2, 'b']], columns=['col_1', 'col_1']) | ||
|
|
||
| NON_UNIQUE_COLUMN_MESSAGE = ( |
There was a problem hiding this comment.
🐄 no real need for this to be a CONSTANT, imo. no biggie though.
There was a problem hiding this comment.
Do you mean rename it to something like pattern or message rather than ALLCAPSCONSTANT?
There was a problem hiding this comment.
expected_message or something is what I meant.
For issue: #665
Just a first pass. Haven't added tests yet - want to make sure my approach is good enough.