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

Add support for API extensions #1617

Merged
merged 11 commits into from
Jul 1, 2020
Merged

Add support for API extensions #1617

merged 11 commits into from
Jul 1, 2020

Conversation

scook12
Copy link
Contributor

@scook12 scook12 commented Jun 27, 2020

This should close #1465 and #1285.

@scook12
Copy link
Contributor Author

scook12 commented Jun 27, 2020

Hmm, looks like 3.5 testing is in the pipe? Failing checks 1 and 2 because of f-strings.

From the docs:

If you are using Conda, the Koalas installation and developement enviornment are as follows.

# Python 3.6+ is required
conda create --name koalas-dev-env python=3.6
...

If 3.5+ support is required, I can rollback and add a to fix the docs, as well.

Rest of the failures (3.6+) are from one the doctests of the testing utility that I uncritically replicated from pandas._testing. Will take a look.

Edit:

3.6+ now passing. The testing utility itself is fine, but the outputs in the pipeline don't match up precisely, so I skipped them.
Doctest:

    >>> with assert_produces_warning(False): # doctest: +SKIP
    ...     warnings.warn(RuntimeWarning())
    ...
    Traceback (most recent call last):
        ...
    AssertionError: Caused unexpected warning(s): ['RuntimeWarning'].

Output in the pipeline that caused original check failures is still AssertionError, just more verbose:

    AssertionError: Caused unexpected warning(s): [('RuntimeWarning', RuntimeWarning(), '/home/runner/work/koalas/koalas/databricks/koalas/testing/utils.py', 2)]

Edit 2:

Fixed for 3.5, as well.

@HyukjinKwon
Copy link
Member

Thanks @scook12 for working on this. Seems fine from a cursory look but let me take a closer look within few days.

@scook12
Copy link
Contributor Author

scook12 commented Jun 29, 2020

Sounds good! Thanks.

databricks/koalas/extensions.py Outdated Show resolved Hide resolved
databricks/koalas/extensions.py Outdated Show resolved Hide resolved
databricks/koalas/extensions.py Outdated Show resolved Hide resolved
databricks/koalas/extensions.py Outdated Show resolved Hide resolved
databricks/koalas/extensions.py Outdated Show resolved Hide resolved
databricks/koalas/tests/test_accessor.py Outdated Show resolved Hide resolved
databricks/koalas/tests/test_accessor.py Outdated Show resolved Hide resolved
@ueshin
Copy link
Collaborator

ueshin commented Jun 29, 2020

Otherwise, LGTM. Thanks!

@scook12
Copy link
Contributor Author

scook12 commented Jun 29, 2020

Alright, think I got 'em all. Let me know if there's any other changes you wanted or if I missed something. Thanks!

Copy link
Collaborator

@ueshin ueshin left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM.
I'd leave this to @HyukjinKwon.

Thanks for working on this!

databricks/koalas/extensions.py Show resolved Hide resolved
databricks/koalas/extensions.py Outdated Show resolved Hide resolved
"""
Register a custom accessor on {klass} objects.

Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe underline is missing? :)

Parameters
----------

name : str
name used when calling the accessor after its registered

Returns
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: indent


See Also
--------
register_dataframe_accessor: Register a custom accessor on Series objects
Copy link
Contributor

Choose a reason for hiding this comment

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

Series objects -> DataFrame objects ?


See Also
--------
register_dataframe_accessor: Register a custom accessor on Series objects
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

See Also
--------
register_dataframe_accessor: Register a custom accessor on Series objects
register_series_accessor: Register a custom accessor on Index objects
Copy link
Contributor

Choose a reason for hiding this comment

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

Index objects -> Series objects ?

@itholic
Copy link
Contributor

itholic commented Jun 30, 2020

Otherwise, LGTM 👍

@scook12
Copy link
Contributor Author

scook12 commented Jun 30, 2020

Thanks for all the code review @ueshin and @itholic! I think (hope) I've killed the last of the typos. :)

@HyukjinKwon
Copy link
Member

This is awesome. Thanks @scook12. Merged.

@HyukjinKwon HyukjinKwon merged commit 93ce3e8 into databricks:master Jul 1, 2020
@scook12
Copy link
Contributor Author

scook12 commented Jul 2, 2020

Great! thanks @HyukjinKwon, happy to do it.

@itholic itholic mentioned this pull request Sep 1, 2020
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.

Support for API Extensions
4 participants