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

Add Marc21XmlEncoder #531

Closed
wants to merge 9 commits into from
Closed

Conversation

dr0i
Copy link
Member

@dr0i dr0i commented May 6, 2024

Resolves #527.

Marc21XmlEncoder acts as a wrapper. It makes use of Marc21Encoder, Marc21Decoder and MarcXmlEncoder to ensure a proper MarcXml, especially regarding the leader. Also - in contrast to MarcXmlEncoder - the record id (field 001) is mandatory.

Marc21XmlEncoder acts as a wrapper. It makes use of Marc21Encoder, Marc21Decoder
and MarcXmlEncoder to ensure a proper MarcXml, especially regarding the leader.
Also - in contrast to MarcXmlEncoder - the record id (field 001) is mandatory.
@dr0i
Copy link
Member Author

dr0i commented May 6, 2024

There may be a better way of implementing this. Probably at least regarding the tests.

To understand the difference between MarcXmlEncoder and Marc21XmlEncoder see e.g. bff54da#diff-9fb05322904ee7e4b0be892e9c4a42db323e95f617277dfd9ee0d7c233a604fcR227, where issue336_createRecordWithTopLevelLeader_Marc21Xml is expected to fail when computed by MarcXmlEncoder but fine when using Marc21XmlEncoder .

Another difference is that Marc21XmlEncoder expects the existence of field 001 (aka record id).

@dr0i dr0i requested a review from blackwinter May 6, 2024 14:52
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.

I think having both encode-marcxml and encode-marc21xml might be confusing. I would have preferred an option on encode-marcxml (whose default could be flipped with the next major version). But if you feel that it's too complicated to implement, I'm okay with it.

@blackwinter blackwinter assigned dr0i and unassigned blackwinter May 7, 2024
@dr0i dr0i force-pushed the 527-emitLeaderAlwaysAsOneStringWrapper branch from 38d0d2e to dc81abe Compare May 8, 2024 10:16
dr0i added a commit that referenced this pull request May 13, 2024
@dr0i dr0i force-pushed the 527-emitLeaderAlwaysAsOneStringWrapper branch from dc81abe to c7f0c28 Compare May 13, 2024 11:51
dr0i added a commit that referenced this pull request May 13, 2024
@dr0i dr0i force-pushed the 527-emitLeaderAlwaysAsOneStringWrapper branch from c7f0c28 to a74d265 Compare May 13, 2024 11:57
dr0i added a commit that referenced this pull request May 13, 2024
@dr0i dr0i force-pushed the 527-emitLeaderAlwaysAsOneStringWrapper branch from a74d265 to 3611be6 Compare May 13, 2024 11:58
@dr0i dr0i force-pushed the 527-emitLeaderAlwaysAsOneStringWrapper branch from 3611be6 to e75fd57 Compare May 13, 2024 12:20
@dr0i
Copy link
Member Author

dr0i commented May 13, 2024

With fde6ba1 comes the incorporation of the new feature into MarcXmlEncoder. Note, though, that this makes MarcXmlEncoder a singleton (to avoid recursion ad infinitum) and thus, if it comes to threads, a) disables any performance gains ? b) is not thread safe?
Also note the test ensureCorrectMarc21XmlParameterAfterSettingReceiver which throws a NPE when setting the ensureCorrectMarc21Xml after setting the receiver of the pipeline (which cannot happen using flux but can happen when using the Java-API).
@blackwinter you may have a better idea re implementation

@dr0i dr0i assigned blackwinter and unassigned dr0i May 13, 2024
The outer class is responsible for routing the stream into the appropriate pipe, the inner class contains the actual (previous) implementation.
@blackwinter
Copy link
Member

Not sure if this is so much better, but it may be helpful to separate the concerns more clearly. See c1f8f98 (with some additional fixes/cleanup).

@blackwinter blackwinter assigned dr0i and unassigned blackwinter May 14, 2024
@dr0i
Copy link
Member Author

dr0i commented May 24, 2024

Thx - your changes are very good, i.a. the encoder is threadable again 👍

@blackwinter
Copy link
Member

Superseded by #538 and #539.

@blackwinter blackwinter deleted the 527-emitLeaderAlwaysAsOneStringWrapper branch June 19, 2024 13:35
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.

encode-marcxml should combine split leader fields of decode-marc21 to create valid marc xml
2 participants