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

docs: added in-memory caching to reference and concepts sections #1211

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented Oct 30, 2024

Title says it all!

@zilto zilto added the documentation Improvements or additions to documentation label Oct 30, 2024
@DAGWorks-Inc DAGWorks-Inc deleted a comment from sweep-ai bot Oct 30, 2024
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 everything up to 7194fcc in 1 minute and 1 seconds

More details
  • Looked at 141 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. docs/concepts/caching.rst:505
  • Draft comment:
    The import statement for ShelveResultStore is incorrect. It should be FileResultStore. Please update the import path accordingly.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is no longer relevant because the suggested change has already been implemented in the diff. The import statement now correctly uses FileResultStore, so the comment should be removed.
    I might be missing some context about why the comment was made initially, but based on the current state of the code, the issue has been resolved.
    The diff clearly shows that the import statement has been corrected, so the comment is outdated and should be removed.
    Remove the comment because the import statement issue has already been resolved in the diff.
2. docs/concepts/caching.rst:505
  • Draft comment:
    from hamilton.caching.stores.file import FileResultStore
  • Reason this comment was not posted:
    Marked as duplicate.
3. docs/concepts/caching.rst:578
  • Draft comment:
    Duplicate functionality found in hamilton/caching/stores/memory.py. Consider using the existing InMemoryResultStore class instead of adding a new one.

  • memory.py

  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests that the InMemoryResultStore class is being duplicated, but the diff shows that this class is being used, not redefined. The comment might be incorrect if it assumes a redefinition that isn't present in the diff. I need to verify if the comment is about a change made in this diff or if it's a misunderstanding.
    I might be missing the context of the entire codebase, which could affect the accuracy of my assessment. The comment might be referring to a broader issue not visible in the diff.
    The comment should be about changes in the diff, and if it refers to something outside the diff, it might not be relevant for this review.
    The comment seems to be incorrect as it suggests a duplication that isn't evident in the diff. It should be removed unless there's clear evidence of duplication in the changes made.

Workflow ID: wflow_6IUjnyechdgomg3E


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@zilto zilto merged commit afc062c into main Oct 30, 2024
24 checks passed
@zilto zilto deleted the docs/in-memory-caching branch October 30, 2024 19:16
jernejfrank pushed a commit to jernejfrank/hamilton that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants