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

fix: ibis extension registration #737

Merged
merged 4 commits into from
Mar 5, 2024
Merged

fix: ibis extension registration #737

merged 4 commits into from
Mar 5, 2024

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented Mar 5, 2024

Dataframe extensions need to register functions:

  • get_column_EXTENSION
  • register_types
  • fill_with_scalar_EXTENSION

Changes

  • a placeholder for fill_with_scalar_EXTENSION was added

Ellipsis 🚀 This PR description was created by Ellipsis for commit 5442f0b.

Summary:

This PR adds and modifies the fill_with_scalar_ibis function in ibis_extensions.py, registering it with the fill_with_scalar registry and indicating that filling Ibis columns with scalars is not yet supported.

Key points:

  • Added and modified the fill_with_scalar_ibis function in ibis_extensions.py.
  • Registered fill_with_scalar_ibis with the fill_with_scalar registry.
  • The function raises a NotImplementedError, indicating that filling Ibis columns with scalars is not supported.

Generated with ❤️ by ellipsis.dev

ellipsis-dev[bot]

This comment was marked as spam.

@zilto zilto requested review from skrawcz and elijahbenizzy March 5, 2024 16:47
@zilto zilto added the bug Something isn't working label Mar 5, 2024
Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

Add instructions on what to do if you hit this error (+ maybe related github issue?). Should be part of the error message.

ellipsis-dev[bot]

This comment was marked as spam.

ellipsis-dev[bot]

This comment was marked as spam.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me!

  • Performed an incremental review on 211d47a
  • Looked at 15 lines of code in 1 files
  • Took 1 minute and 25 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. hamilton/plugins/ibis_extensions.py:29:
  • Assessed confidence : 0%
  • Comment:
    The changes made to the fill_with_scalar_ibis function are in line with the PR description and do not seem to introduce any new bugs or violate any best practices.
  • Reasoning:
    The changes in the PR are minimal and seem to be in line with the PR description. The function fill_with_scalar_ibis is already raising a NotImplementedError, and the PR just modifies the way the error message is displayed (splitting it into two lines instead of one). This doesn't seem to introduce any new bugs or violate any best practices.

Workflow ID: wflow_gVdNLA9rvas8ZsZ2


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

@zilto zilto merged commit 5442f0b into main Mar 5, 2024
23 checks passed
@zilto zilto deleted the fix/extension-registry branch March 5, 2024 17:13
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me!

  • Performed an incremental review on 5442f0b
  • Looked at 24 lines of code in 1 files
  • Took 1 minute and 30 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. hamilton/plugins/ibis_extensions.py:28:
  • Assessed confidence : 0%
  • Comment:
    The placeholder function and its registration look good. The use of NotImplementedError is a good practice for placeholder functions.
  • Reasoning:
    The PR seems to be in order. The author has added a placeholder function for 'fill_with_scalar_ibis' and registered it. The function raises a NotImplementedError, which is a good practice for placeholder functions. The code is clean, follows the DRY principle, and doesn't contain any secrets or credentials. The function and method naming also follow consistent patterns. There are no apparent logical, performance, or security bugs. The PR description is clear and matches the changes made.

Workflow ID: wflow_8GrjwnthDp6WKLF5


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants