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 "batch-reset creates empty records" #560

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

dr0i
Copy link
Member

@dr0i dr0i commented Sep 27, 2024

Fixes #543.

@dr0i
Copy link
Member Author

dr0i commented Sep 27, 2024

functional review : @TobiasNx (see #543 (comment) for a useful flux)
code review: @blackwinter

@dr0i dr0i requested a review from TobiasNx September 27, 2024 14:59
Copy link
Member

@blackwinter blackwinter left a comment

Choose a reason for hiding this comment

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

Please don't add failing tests. Every commit should pass on its own.

Either introduce the tests along with their fix, or add tests showing the wrong behaviour (including a comment to that effect) and then fix them later along with the implementation.

Note that we introduced the @MetafixToDo annotation in metafacture-fix for exactly this purpose (see metafacture/metafacture-fix#158).

@blackwinter blackwinter assigned dr0i and unassigned blackwinter Sep 27, 2024
By not calling the pipe (aka wrapper) but the receiver directly
the stream is only once resetted when called once.

(In conjunction with ObjectFileWriter and StreamBatchResetter this bug
had resulted in as many empty files as non-empty ones.)
@dr0i dr0i force-pushed the 543-fixBatchResetCreatesEmptyRecords branch from 16b9349 to 3724a99 Compare September 30, 2024 08:26
@dr0i dr0i force-pushed the 543-fixBatchResetCreatesEmptyRecords branch from 3724a99 to d3a47ee Compare September 30, 2024 08:28
@dr0i dr0i assigned TobiasNx and unassigned dr0i Sep 30, 2024
TobiasNx added a commit to TobiasNx/notWorkingFlux that referenced this pull request Oct 2, 2024
 metafacture/metafacture-core#560

Commit-Hash: d3a47eeb70166d1ffe66ffd1cf7f3926dbc89c4d
Copy link
Contributor

@TobiasNx TobiasNx left a comment

Choose a reason for hiding this comment

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

+1 will open a new ticket for the empty record at the end

@TobiasNx TobiasNx assigned dr0i and unassigned TobiasNx Oct 17, 2024
@dr0i dr0i merged commit bdb93ad into master Oct 17, 2024
1 check passed
@dr0i dr0i deleted the 543-fixBatchResetCreatesEmptyRecords branch October 17, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

batch-reset creates an empty record at the end when writing and many empty records with encode marcxml
3 participants