search

metafacture / metafacture-core

Core package of the Metafacture tool suite for metadata processing.
https://metafacture.org
Apache License 2.0
71 stars 34 forks source link

Make emitting MARCXML namespace configurable. #410

Closed blackwinter closed 2 years ago

blackwinter commented 2 years ago

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.

blackwinter commented 2 years ago

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.

dr0i commented 2 years ago

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?

blackwinter commented 2 years ago

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 commented 2 years ago
dr0i commented 2 years ago

Thx - good explanation! I agree.