Skip to content

Conversation

@blackwinter
Copy link
Member

This is (presumably) the least disruptive modification to achieve the goal, but there's probably potential to overhaul the XML construction (and to reuse constants in the test class).

Fixes #403.

- Open and close tag are derived from a single definition.
- Writing call sites are simplified (`writeRaw` -> `writeTag`).
@blackwinter
Copy link
Member Author

I've added a commit to simplify XML tag handling a little. If you don't like it, I'll revert.

Whether any logic (even constants) should be reused in the test class, is actually debatable. I've decided against it for now.

Copy link
Member

@dr0i dr0i left a comment

Choose a reason for hiding this comment

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

I am fine with it, also the deeper code is more complex. So +1.

@dr0i
Copy link
Member

dr0i commented Oct 21, 2021

Whether any logic (even constants) should be reused in the test class, is actually debatable.

Let's debate (a bit).
I personally see this quite the opposite. (I am going so far to favour "real" (integration-) tests having "real" input data (as far as these don't involve networks) matching again expected output. But I am good with it as it is here, also because e.g. these tests would slow down build time tremendeously).
So, coming back to the issue - don't you agree in general that testing as more code (including constants) as possible is better?

@dr0i dr0i assigned blackwinter and unassigned dr0i Oct 21, 2021
@blackwinter
Copy link
Member Author

Hm, maybe there's been a misunderstanding...

don't you agree in general that testing as more code (including constants) as possible is better?

Absolutely! Ideally, the complete (public) interface would be covered by tests. But we shouldn't rely on any part of this interface in order to verify its behaviour. That's what I meant by "reusing logic (or constants) in the test class".

E.g., it might be convenient to construct expected values in a test by utilizing the MarcXmlEncoder.Tag enum. But then we wouldn't notice any errors in its implementation.

Similarly, it might seem obvious to reference, e.g., the MarcXmlEncoder.NAMESPACE constant in a test. But then we wouldn't notice when it gets changed (intentionally or not).

@blackwinter
Copy link
Member Author

blackwinter commented Oct 21, 2021

@dr0i
Copy link
Member

dr0i commented Oct 21, 2021

Thx - good explanation! I agree.

@blackwinter blackwinter merged commit 533b5c3 into master Oct 27, 2021
@blackwinter blackwinter deleted the 403-makeMarcXmlEncoderNamespaceConfigurable branch October 27, 2021 08:37
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.

Marc XML-encoder-Output: Option with and without namespace

3 participants