Skip to content

Conversation

sungwy
Copy link
Collaborator

@sungwy sungwy commented May 2, 2025

Rationale for this change

Nit: the current order (to_snapshot before from_snapshot) is unexpected and error-prone.

This PR proposes to reorder these parameters in validation_history and ancestors_between so that it is more idiomatic.

Are these changes tested?

Yes, through integration and unit tests

Are there any user-facing changes?

Yes, but these functions were just introduced (no releases were made yet with these functions)

@sungwy sungwy requested a review from Fokko May 2, 2025 01:52
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks @sungwy for making this change, this is also more compatible with my mind.

This has never been released, so it is okay to update it now 👍 Moving forward 🚀

@Fokko Fokko merged commit 34c8949 into apache:main May 2, 2025
7 checks passed
gabeiglio pushed a commit to Netflix/iceberg-python that referenced this pull request Aug 13, 2025
…1959)

<!--
Thanks for opening a pull request!
-->

<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->

# Rationale for this change

Nit: the current order (`to_snapshot` before `from_snapshot`) is
unexpected and
[error-prone](https://github.com/apache/iceberg-python/pull/1938/files#r2070985497).

This PR proposes to reorder these parameters in `validation_history` and
`ancestors_between` so that it is more idiomatic.
# Are these changes tested?

Yes, through integration and unit tests

# Are there any user-facing changes?

Yes, but these functions were just introduced (no releases were made yet
with these functions)
<!-- In the case of user-facing changes, please add the changelog label.
-->
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