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 issue with asdftool diff #1652

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Fix issue with asdftool diff #1652

merged 3 commits into from
Sep 29, 2023

Conversation

braingram
Copy link
Contributor

@braingram braingram commented Sep 26, 2023

EDIT: also fixes the changelog moving entries that erroneously were added to 2.15.2 following rebasing of now merged PRs.

#1537 introduced a bug in asdftool diff where comparisons between two arrays that differ only by contents (and not by shape, dtype, etc) were considered identical.

This PR fixes that issue and introduces a test for the regression test_diff_ndarray (and data files for this test).

Additionally, this PR updates the output of asdftool diff to report which arrays differ. The changes above and this new output can be illustrated by comparing the output for the newly included test files ndarray0.asdf and ndarray1.asdf.

Comparing these with asdf 2.15 produces:

    ndarrays differ by contents

The lack of the keys to find the array that differs is here considered a bug.
Running the same comparison with current asdf main produces no output which is certainly a bug.
With this PR the output is now:

    ndarrays at "a" differ by contents

As the bug introduced in #1537 has not yet been released the changelog entry only mentions the change in the diff output to include the array name.

@braingram braingram requested a review from a team as a code owner September 26, 2023 18:30
@braingram braingram requested a review from eslavich September 26, 2023 18:50
@braingram braingram added this to the 3.0.0 milestone Sep 26, 2023
compare 2 files that are identical except for
block contents
Copy link
Contributor

@eslavich eslavich left a comment

Choose a reason for hiding this comment

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

LGTM. Nicer output for sure.

@braingram braingram merged commit 79356ab into asdf-format:main Sep 29, 2023
29 of 30 checks passed
@braingram braingram deleted the diff branch September 29, 2023 20:08
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