#3 ignore namespaces

[iyfedorov]

Hello! I didn't fully understand quasiquotes but can you look at it?

[valentiay]

Hi! Thanks for your contribution!

I like the idea of modifying Cursor. It's good in its simplicity. The solution I was thinking of was much more complicated.

I've left some minor comments in the code, but there's also one major thing to consider. Look at this code sample:

import phobos.decoding.ElementDecoder
import phobos.decoding.XmlDecoder
import phobos.derivation.semiauto.deriveXmlDecoderConfigured
import phobos.configured.ElementCodecConfig
import phobos.derivation.semiauto.deriveElementDecoder
import phobos.syntax.xmlns

case object com
implicit val comNamespace: Namespace[com.type] = Namespace.mkInstance("example.com")

final case class Bar(@xmlns(com) qux: String)
// Default config with ignoreNamespaces=false is used
implicit val barDecoder: ElementDecoder[Bar] = deriveElementDecoder

val fooConfig = ElementCodecConfig.default.withIgnoreNamespaces()
final case class Foo(baz: String, @xmlns(com) bar: Bar)
implicit val fooDecoder: XmlDecoder[Foo] = deriveXmlDecoderConfigured("foo", fooConfig)

val string =
  """<com:foo xmlns:com="example.com">
    |  <com:baz>test2</com:baz><!-- Namespace is reset here -->
    |  <com:bar><com:qux>test1</com:qux></com:bar> <!-- But namespace is not reset here -->
    |</com:foo>
    |""".stripMargin

println(fooDecoder.decode(string))
// Right(Foo(test2,Bar(test1)))

Here, "ignoreNamespaces" applies to "baz" (because it uses a primitive string decoder), but not to "bar" (because it uses derived decoder with default configuration). This does not cause any bugs, but it is definitely counter-intuitive and can also be inconvenient.

I think that "ignoreNamespaces" should work like "scopeDefaultNamespace", which affects all elements within the current one. If "ignoreNamespaces" is made an optional setting, then it will be possible to implement this behavior with a stack within "Cursor", just as for "scopeDefaultNamespace". This way the behaviour would not only be more intuitive but you would also not have to set "ignoreNamespace" for every derived decoder.

[valentiay]

I believe this test should be in DecoderDerivationTest.scala

[valentiay]

This setting needs a little more documentation. It actually does not ignore namespaces, but it makes all namespaces empty. Therefore, decoding may still fail if a namespace is specified for some inner elements.

Maybe it would be better to rename this to "removeNamespaces" or something similar.

[valentiay]

It may be more convenient to remove this parameter from the method and change its implementation to copy(ignoreNamespaces=true), as false is the default option

[iyfedorov]

Hi! Thanks for your contribution!

I like the idea of modifying Cursor. It's good in its simplicity. The solution I was thinking of was much more complicated.

I've left some minor comments in the code, but there's also one major thing to consider. Look at this code sample:

import phobos.decoding.ElementDecoder
import phobos.decoding.XmlDecoder
import phobos.derivation.semiauto.deriveXmlDecoderConfigured
import phobos.configured.ElementCodecConfig
import phobos.derivation.semiauto.deriveElementDecoder
import phobos.syntax.xmlns

case object com
implicit val comNamespace: Namespace[com.type] = Namespace.mkInstance("example.com")

final case class Bar(@xmlns(com) qux: String)
// Default config with ignoreNamespaces=false is used
implicit val barDecoder: ElementDecoder[Bar] = deriveElementDecoder

val fooConfig = ElementCodecConfig.default.withIgnoreNamespaces()
final case class Foo(baz: String, @xmlns(com) bar: Bar)
implicit val fooDecoder: XmlDecoder[Foo] = deriveXmlDecoderConfigured("foo", fooConfig)

val string =
  """<com:foo xmlns:com="example.com">
    |  <com:baz>test2</com:baz><!-- Namespace is reset here -->
    |  <com:bar><com:qux>test1</com:qux></com:bar> <!-- But namespace is not reset here -->
    |</com:foo>
    |""".stripMargin

println(fooDecoder.decode(string))
// Right(Foo(test2,Bar(test1)))

Here, "ignoreNamespaces" applies to "baz" (because it uses a primitive string decoder), but not to "bar" (because it uses derived decoder with default configuration). This does not cause any bugs, but it is definitely counter-intuitive and can also be inconvenient.

I think that "ignoreNamespaces" should work like "scopeDefaultNamespace", which affects all elements within the current one. If "ignoreNamespaces" is made an optional setting, then it will be possible to implement this behavior with a stack within "Cursor", just as for "scopeDefaultNamespace". This way the behaviour would not only be more intuitive but you would also not have to set "ignoreNamespace" for every derived decoder.

It`s true. Propagation strategy will be more convenient. I will try to add this

[valentiay]

Thanks. Now everything looks good to me. I will merge, when pipeline succeeds