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

Sum should work with time deltas, also isinstance cleanups #735

Merged
merged 2 commits into from
Mar 31, 2020
Merged

Sum should work with time deltas, also isinstance cleanups #735

merged 2 commits into from
Mar 31, 2020

Conversation

dannysepler
Copy link

@dannysepler dannysepler commented Mar 15, 2020

hi! i found this a compelling issue to look at wireservice/csvkit#1029

given i'm pretty new to open source (as well as agate), i was wondering your opinion on a few things:

  1. besides running unit tests, what would you imagine would be a good testing plan here?
  2. my additions broke the "pivot" unit tests in the following way (which i resolved in this PR):
___ TestPivot.test_pivot_sum ___
self = <tests.test_table.test_pivot.TestPivot testMethod=test_pivot_sum>
    def test_pivot_sum(self):
        table = Table(self.rows, self.column_names, self.column_types)
>       pivot_table = table.pivot('race', 'gender', Sum('age'))
tests/test_table/test_pivot.py:120:
_ _ _ 
agate/table/pivot.py:113: in pivot
    column_type = aggregation.get_aggregate_data_type(groups)
_ _ _
self = <agate.aggregations.sum.Sum object at 0x101717f90>, table = <agate.tableset.TableSet object at 0x10171db40>
    def get_aggregate_data_type(self, table):
>       column = table.columns[self._column_name]
E       AttributeError: 'TableSet' object has no attribute 'columns'

this seems to be because we get the data type from "groups" rather than the whole table. is this a good patch or not really?

  1. would you prefer i break out the isinstance cleanups into another diff?

thanks all!

@dannysepler
Copy link
Author

bump? @jpmckinney maybe?

@jpmckinney
Copy link
Member

Thanks!

  1. The new test seems sufficient.
  2. It doesn't break existing tests, so assuming agate has good coverage, seems fine.
  3. It'd be better to split out into separate PR, but it's no problem this time.

Travis passes on 2.7 and 3.6, but is failing on EOL / soon-to-be EOL versions 3.4 and 4.5, and PyPi is in general a bit busted on Travis. So, merging!

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.

2 participants