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

TTO-207 investigate discrepancy in hathifiles full vs. incremental #42

Merged
merged 2 commits into from
May 1, 2024

Conversation

moseshll
Copy link
Contributor

@moseshll moseshll commented Apr 30, 2024

cutoff variable used in generate_hathifile.rb is currently excluding HTIDs that arguably should be treated as modified due to changes in the host catalog record. This patch gets rid of that variable and treats all HTIDs on the record as having been affected by the record change (thus including them in the resulting hathifile) regardless of their 974(d) update date.

Tests should be refactored at some point but I consider it out of scope for a TTO issue.

Includes a completely unrelated added test for update_hathifile_listing.rb (it "removes existing files that are too old" do ...) since Coveralls got irked at the slight decrease in test coverage due to the code deletion that is the main focus of this PR. Are you happy now, Coveralls??

`cutoff` variable used in `generate_hathifile.rb` was excluding HTIDs that arguably changed due
to changes in the catalog record. This patch gets rid of that variable and treats all HTIDs
as having been affected by the record change (thus including them in the resulting hathifile)
regardless of their 974(d) update date.

Tests should be refactored at some point but I consider it out of scope for a TTO issue.
@coveralls
Copy link

coveralls commented Apr 30, 2024

Coverage Status

coverage: 100.0% (+0.2%) from 99.791%
when pulling 42eda59 on TTO-207_remove_cutoff
into 756c6e4 on main.

@moseshll moseshll requested a review from mwarin May 1, 2024 03:05
Copy link
Contributor

@mwarin mwarin 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.

You say:

Tests should be refactored at some point

Make an issue or a ticket so we don't (immediately) forget?

APPROVE.

@moseshll
Copy link
Contributor Author

moseshll commented May 1, 2024

Refactoring issue added as suggested. Along with the all-important "DRY" badge for future issues!

@moseshll moseshll merged commit 4753d62 into main May 1, 2024
2 checks passed
@moseshll moseshll deleted the TTO-207_remove_cutoff branch May 1, 2024 14:34
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.

3 participants