search

mlabs-haskell / opc-xml-da-client

OPC XML-DA Client for Haskell
MIT License
2 stars 1 forks source link

Supply XML types, manual Hashable instances #2

Closed kozross closed 3 years ago

kozross commented 3 years ago

I've left the wrappers intact name-wise, because I believe them to be clearer. This also avoids orphans by deriving everything via the Generic instances in xml-types, but this is very slow-compiling. We could either hand-roll all the hashing without Generic (tedious, error-prone and slow) or we can send a PR to xml-types.

nikita-volkov commented 3 years ago

I don't think that the compilation speed is an important issue now. Let's just get it to work ASAP, leaving all the refining for the next phase of work.

Regarding the code changes, I have two comments:

  1. Please notice that in the Prelude module the autoformatting has messed up the positioning of import-sectioning comments, which were intended for easier discovery by package. Let's either restore the order or make a decision to just drop those comments altogether.

  2. The XmlValue name is weird. Element is the standard way to call it, it's used in DOM and in many places of XML documentation. E.g., here it is on Wikipedia: https://en.wikipedia.org/wiki/XML#Element

kozross commented 3 years ago

I would advocate for dropping the comments. For naming, I'm happy to change to XmlElement, as we'll need a wrapper anyway for providing missing instances.

nikita-volkov commented 3 years ago

Great. Approved then!