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

Updates docstring of data adapters to be public facing #752

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

elijahbenizzy
Copy link
Collaborator

@elijahbenizzy elijahbenizzy commented Mar 8, 2024

Quick docstring update

Changes

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Ellipsis 🚀 This PR description was created by Ellipsis for commit cbdca27.

Summary:

This PR updates the docstrings of DataLoader and DataSaver classes in hamilton/io/data_adapters.py to reflect their public-facing status and increments the Hamilton library version to 1.53.0rc0.

Key points:

  • Updated docstrings of DataLoader and DataSaver classes in hamilton/io/data_adapters.py.
  • Removed notes about these classes not being public-facing APIs.
  • Updated Hamilton library version to 1.53.0rc0 in hamilton/version.py.

Generated with ❤️ by ellipsis.dev

This is now fully public-facing, we just had not updated the docstring
to show it.
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!

  • Reviewed the entire pull request up to cbdca27
  • Looked at 39 lines of code in 2 files
  • Took 37 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/io/data_adapters.py:115:
  • Assessed confidence : 0%
  • Comment:
    The removal of the comments makes sense as the API is now public-facing. The changes are clear and do not introduce any issues.
  • Reasoning:
    The changes in this PR are straightforward and involve only the removal of some comments and a version update. The removed comments were warnings about the API not being public-facing, which is no longer the case. The version update is standard for a new release. There are no logical, performance, or security issues introduced by these changes.

Workflow ID: wflow_WyKtc8Kzvo0oxhRg


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

Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

just don't commit the version.

@@ -1 +1 @@
VERSION = (1, 52, 0)
VERSION = (1, 53, 0, "rc0")
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

@elijahbenizzy elijahbenizzy merged commit e37abe2 into main Mar 9, 2024
23 checks passed
@elijahbenizzy elijahbenizzy deleted the data-adapters-public-docstring branch March 9, 2024 04:53
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