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

refactor: remove ibis NA #9323

Closed
wants to merge 53 commits into from
Closed

Conversation

ncclementi
Copy link
Contributor

@ncclementi ncclementi commented Jun 6, 2024

This PR removes ibis.NA, replacing it appearances for ibis.null()

Merge after #9300 - I'll rebase based on main once merged

Copy link
Contributor

github-actions bot commented Jun 6, 2024

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

@ncclementi ncclementi changed the title refactor: remove ibis.NA refactor: remove ibis NA Jun 6, 2024
@ncclementi
Copy link
Contributor Author

I accidentally change the html from a doc freeze, that had a ibis.NA what's the best way to fix that? I've never generated a frozen page for docs.

@jcrist
Copy link
Member

jcrist commented Jun 7, 2024

Copying a question from the issue into the PR:

For implementation purposes, do we want to deprecate ibis.NAor remove it as a breaking change. If it's a deprecation, how do we deprecate ibis.NA = null() the @deprecated seems to only work for methods/functions.

I think most users never touch ibis.NA, so I wouldn't be opposed to doing a clean break. In that case though I'd think we'd want to wait until 10.0 since that would definitely be a breaking change. Given fillna/dropna are cleanly deprecated in the other PR, I have a slight preference for adding a warning here as well so they all can be released at the same time.

Two ways I can see to do a cleaner deprecation:

  • Expose ibis.NA via a module __getattr__ on the ibis namespace, and warn when that attribute is accessed. This would warn every time ibis.NA or from ibis import NA is executed. The __getattr__ here would just return the result of ibis.null(), the warning is on access not implementation.
  • Create a new NAScalar type (like the existing NullScalar) that warns in the .op() method before returning the existing op representation. This would warn every time ibis.NA is used to build up larger expressions.

I think the former is a better deprecation method, but either would work well enough.

@@ -125,3 +128,18 @@ def connect(*args, **kwargs):
setattr(proxy, name, getattr(backend, name))

return proxy


def __getattr__(name: str) -> Any:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we already had a __getattr__ I went this route to keep the typing for the load_backend portion. Once we deprecate NA completely we can remove the workaround

@ncclementi
Copy link
Contributor Author

I'll rebase here once we merged #9300

Comment on lines +141 to +143
import ibis

return ibis.null()
Copy link
Member

Choose a reason for hiding this comment

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

I think this should work instead, since null is already imported above:

Suggested change
import ibis
return ibis.null()
return null()

Comment on lines +136 to +137
"Accessing 'ibis.NA' is deprecated as of v9.1 and will be removed in a future version. "
"Use 'ibis.null()' instead.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Accessing 'ibis.NA' is deprecated as of v9.1 and will be removed in a future version. "
"Use 'ibis.null()' instead.",
"The 'ibis.NA' constant is deprecated as of v9.1 and will be removed in a future "
"version. Use 'ibis.null()' instead.",

@ncclementi
Copy link
Contributor Author

The amount of conflicts got a bit nasty, it'll be faster to open a new PR and make it cleaner. I'll put this up in a few min.

@ncclementi
Copy link
Contributor Author

Closing in favor of #9344

@ncclementi ncclementi closed this Jun 10, 2024
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.

refactor: remove ibis.NA
2 participants