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

BUG: Allow default config option to be None #738

Closed

Conversation

charlesdong1991
Copy link
Contributor

@charlesdong1991 charlesdong1991 commented Sep 2, 2019

In current setup, when using set_option, it will first check if the given value has the same type of default value. In this way, if the default is set to None which will result in NoneType in the type checking and thus fail to use set_option.

And because of this, the PR #737 will fail, if this PR is merged, the #737 will be changed to adopt this.

@softagram-bot
Copy link

Softagram Impact Report for pull/738 (head commit: 5879370)

⭐ Change Overview

Showing the changed files, dependency changes and the impact - click for full size
(Open in Softagram Desktop for full details)

📄 Full report

Impact Report explained. Give feedback on this report to [email protected]

@charlesdong1991 charlesdong1991 changed the title Allow default config option to be None BUG: Allow default config option to be None Sep 2, 2019
@codecov-io
Copy link

codecov-io commented Sep 2, 2019

Codecov Report

Merging #738 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #738      +/-   ##
==========================================
+ Coverage    94.4%   94.41%   +<.01%     
==========================================
  Files          32       32              
  Lines        5580     5584       +4     
==========================================
+ Hits         5268     5272       +4     
  Misses        312      312
Impacted Files Coverage Δ
databricks/koalas/config.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b876400...5879370. Read the comment docs.

@HyukjinKwon
Copy link
Member

Let's track it here #739

@HyukjinKwon HyukjinKwon closed this Sep 3, 2019
HyukjinKwon added a commit that referenced this pull request Sep 4, 2019
…739)

This PR takes over #738, and this fix was inspired by the PR. So the commit credits to, and is authored by @charlesdong1991.

This PR adds `Option` class to allow various cases such as type checking, value checking, etc.

See the usage below as a example

```python
Option(
    key='compute.default_index_type',
    doc=(
        "This sets the default index type: sequence, distributed and distributed-sequence."),
    default='sequence',
    types=str,
    check_func=(
        lambda v: v in ('sequence', 'distributed', 'distributed-sequence'),
        "Index type should be one of 'sequence', 'distributed', 'distributed-sequence'."))
```

While I am here, I also fixed `plotting.sample_ratio` respects `plotting.max_rows` by default if this is not set.

Also, let `DataFrame.transform` respect `compute.max_rows` as well with minor cleanups.
HyukjinKwon pushed a commit that referenced this pull request Sep 4, 2019
…739)

This PR takes over #738, and this fix was inspired by the PR. So the commit credits to, and is authored by @charlesdong1991.

This PR adds `Option` class to allow various cases such as type checking, value checking, etc.

See the usage below as a example

```python
Option(
    key='compute.default_index_type',
    doc=(
        "This sets the default index type: sequence, distributed and distributed-sequence."),
    default='sequence',
    types=str,
    check_func=(
        lambda v: v in ('sequence', 'distributed', 'distributed-sequence'),
        "Index type should be one of 'sequence', 'distributed', 'distributed-sequence'."))
```

While I am here, I also fixed `plotting.sample_ratio` respects `plotting.max_rows` by default if this is not set.

Also, let `DataFrame.transform` respect `compute.max_rows` as well with minor cleanups.
rising-star92 added a commit to rising-star92/databricks-koalas that referenced this pull request Jan 27, 2023
…#739)

This PR takes over databricks/koalas#738, and this fix was inspired by the PR. So the commit credits to, and is authored by @charlesdong1991.

This PR adds `Option` class to allow various cases such as type checking, value checking, etc.

See the usage below as a example

```python
Option(
    key='compute.default_index_type',
    doc=(
        "This sets the default index type: sequence, distributed and distributed-sequence."),
    default='sequence',
    types=str,
    check_func=(
        lambda v: v in ('sequence', 'distributed', 'distributed-sequence'),
        "Index type should be one of 'sequence', 'distributed', 'distributed-sequence'."))
```

While I am here, I also fixed `plotting.sample_ratio` respects `plotting.max_rows` by default if this is not set.

Also, let `DataFrame.transform` respect `compute.max_rows` as well with minor cleanups.
rising-star92 added a commit to rising-star92/databricks-koalas that referenced this pull request Jan 27, 2023
…#739)

This PR takes over databricks/koalas#738, and this fix was inspired by the PR. So the commit credits to, and is authored by @charlesdong1991.

This PR adds `Option` class to allow various cases such as type checking, value checking, etc.

See the usage below as a example

```python
Option(
    key='compute.default_index_type',
    doc=(
        "This sets the default index type: sequence, distributed and distributed-sequence."),
    default='sequence',
    types=str,
    check_func=(
        lambda v: v in ('sequence', 'distributed', 'distributed-sequence'),
        "Index type should be one of 'sequence', 'distributed', 'distributed-sequence'."))
```

While I am here, I also fixed `plotting.sample_ratio` respects `plotting.max_rows` by default if this is not set.

Also, let `DataFrame.transform` respect `compute.max_rows` as well with minor cleanups.
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.

4 participants