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

Added feature to create table directly from Pandas DataFrame #3

Closed
wants to merge 12 commits into from

Conversation

nik849
Copy link
Contributor

@nik849 nik849 commented Mar 28, 2017

Added new method to the api, accepts a pandas DataFrame object and returns a new table object.
screen shot 2017-03-28 at 19 19 02

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.09%) to 96.907% when pulling 1b061b0 on nik849:master into 9050929 on tommyip:master.

@nik849
Copy link
Contributor Author

nik849 commented Mar 29, 2017

Sorry, hadn't thought about the test coverage, I'll sort that out and add it to this.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.09%) to 96.907% when pulling 86f9994 on nik849:master into 9050929 on tommyip:master.

@tommyip
Copy link
Owner

tommyip commented Mar 29, 2017

Hey @nik849 thanks for working on this! I am thinking if it would be better to add a new argument to the new() constructor instead of a new api, like:

def new(headings: Optional[List[Any]] = None,
        data: Optional[List[List[Any]]] = None,
        style: Text = 'default',
        device: Text = 'stdout',
        dataframe: Optional[Pandas.DataFrame] = None) -> TableD:
...

>>> tabled.new(dataframe=new_df)

Looks great otherwise, just need some tests.

@tommyip tommyip self-requested a review March 29, 2017 16:30
@nik849
Copy link
Contributor Author

nik849 commented Mar 30, 2017

@tommyip , great, I will do that, I think it will be a fair bit neater that way. I think it will also be easier to test.

@tommyip
Copy link
Owner

tommyip commented Apr 2, 2017

Looks great, I think the conversion should happen in the api function instead of the TableD constructor, but it is fine otherwise. Just need some test and then I am happy to merge this, I can do that if you like.

@nik849
Copy link
Contributor Author

nik849 commented Apr 2, 2017

Ok, I'll swap those bits round and commit those, could you write the test method? I'm not that familiar with the testing side. I'll do it now, thanks

@tommyip
Copy link
Owner

tommyip commented Apr 3, 2017

Thanks @nik849, merged your changes to e72684d !

@tommyip tommyip closed this Apr 3, 2017
@tommyip tommyip removed their request for review April 3, 2017 16:24
@nik849
Copy link
Contributor Author

nik849 commented Apr 3, 2017

@tommyip, great, thanks!

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

3 participants