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

pkgs-lib.formats.xml: init #342633

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Stunkymonkey
Copy link
Contributor

@Stunkymonkey Stunkymonkey commented Sep 17, 2024

Description of changes

add XML export format. beneficial for https://github.com/NixOS/rfcs/blob/master/rfcs/0042-config-option.md

tested via: nix-build . -A tests.pkgs-lib.formats

sonarr can be improved to have settings e.g.: master...Stunkymonkey:nixpkgs:exportarr-sonarr-test. which can be tested via nix-build . -A nixosTests.prometheus-exporters.exportarr-sonarr

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Stunkymonkey Stunkymonkey marked this pull request as ready for review September 17, 2024 20:46
Comment on lines 521 to 533
type = let
valueType = oneOf [
bool
int
float
str
path
(attrsOf valueType)
(listOf valueType)
] // {
description = "XML value";
};
in valueType;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be honest: i just copied and pasted this from the json... not sure if this is a problem

@Stunkymonkey
Copy link
Contributor Author

not sure where to note the badgerfish-notation.

@h7x4
Copy link
Member

h7x4 commented Sep 17, 2024

I'm wondering whether something could/should be done to ensure we can order attributes properly and still have control of the parent node? With the current implementation, it seems like you can either ensure the order of the child attributes by choosing to use a list, or you can use unordered attributes and be allowed to set "@ asdf" (sorry, accidentally pinged this user...) and "#asdf" properties.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Sep 17, 2024
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Better XML support seems very useful. Thank you ❤️

@@ -517,4 +517,37 @@ rec {
'') {};
};

xml = {}: json {} // {
Copy link
Member

Choose a reason for hiding this comment

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

Is this format suitable for markup-like XML, such as XHTML? Based on the examples here and there, that seems to be an unsolved problem.

I doubt that this is the only and best Nix representation of XML, so I'd prefer a more specific name, like xmltodict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is the other way around: attrsToXml right?

maybe we include the badgerfish notation in the name? like badgerfishToXml?

Copy link
Member

Choose a reason for hiding this comment

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

Only referred to xmltodict as the project name, assuming they didn't name the input format. So they implement the badgerfish format? Then that's great!

I guess we could group representations of XML under an xml attrset, so that it's formats.xml.badgerfish? That way it's more easily discoverable in documentation and the repl.

Copy link
Member

Choose a reason for hiding this comment

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

Or formats.xml.fromBadgerfish.
formats.badgerFishToXml also works. We don't have nesting yet, actually.

Maybe @infinisil has an opinion, as he's started this library.
Otherwise, current name will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am open for suggestions. formats.xml.fromBadgerfish sounds good. but lets wait for feedback from others.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about formats.xml { format = "badgerfish" } (probably the default value)? This seems to be the intent with having the attrset parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented the idea from @ambroisie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @infinisil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i someone is looking for different kinds of formats here is a good list: https://wiki.open311.org/JSON_and_XML_Conversion/

But when searching for implementations on GitHub; xmltodict is the most actively maintained and famous one.

So IMHO it make sense to have this as default, because it is the only thing that has "prevailed"

</child2>
</root>
'';
};
Copy link
Member

Choose a reason for hiding this comment

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

How would namespaces look?

Does definition merging work as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it follows: https://www.xml.com/pub/a/2006/05/31/converting-between-xml-and-json.html:

doesn't take into consideration the following:

  • XML declaration
  • processing instructions
  • explicit handling of namespace declarations
  • XML comments

i found: https://github.com/martinblech/xmltodict/blob/master/README.md#namespace-support

Copy link
Contributor Author

@Stunkymonkey Stunkymonkey Sep 18, 2024

Choose a reason for hiding this comment

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

namespaces are "mainly problematic" for parsing the other way around. it can be achieved via:

{
  "RSS": {
    "@xmlns:jwplayer": "http://support.jwplayer.com/customer/portal/articles/1403635-media-format-reference#feeds";
    "@version": "2.0";
    "Channel": {
...

to create:

<?xml version="1.0" encoding="utf-16"?>
<RSS xmlns:jwplayer="http://support.jwplayer.com/customer/portal/articles/1403635-media-format-reference#feeds" version="2.0">
  <Channel>

Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense.

Handling foreign input while programming with hardcoded prefixes is basically wrong; an XML document should behave exactly the same under prefix renaming, but a lot of software requires certain prefixes, which is - in principle - wrong.
However, we will have to cater to such applications, and users who are kinda ok with such requirements, so that's just that.

Treating any node as URI + tag instead of prefix + tag makes any XML processing more robust (for an option type, the processing means implementing a merge operation for the XML representation), so that may be worth considering, but it seems that we'd end up writing a small library for that purpose; a bit of a research project.

So for now I think we only need to point out in the docs that the type deals with the concrete syntax of the XML, and all responsibility is on the user when it comes to the correctness of any use of namespace features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since this will not be the "perfect/best" xml-exporter for nix, I would suggest to get started, but name it "specially" (not xml as we both agreed above.).

Surely there are a lot of things to improve, but in the end we should enable other users to enable new NixOS-options. Migrating this to other formats seems tedious (manual work), but doable.

Documentation is currently missing completely, because I do not know where to put it. Any suggestions where to put it?

@Stunkymonkey Stunkymonkey changed the title pkgs-lib.formats.xml: init pkgs-lib.formats.badgerfishToXml: init Sep 20, 2024
@Stunkymonkey Stunkymonkey changed the title pkgs-lib.formats.badgerfishToXml: init pkgs-lib.formats.xml: init Oct 7, 2024
@Stunkymonkey Stunkymonkey requested a review from ambroisie October 8, 2024 18:32
@Stunkymonkey
Copy link
Contributor Author

ping @infinisil ? Is this not wanted?

@infinisil
Copy link
Member

infinisil commented Dec 29, 2024

This seems like a fine addition, I'm just very behind on things to review and on holidays (well at least since last week, not since the PR was opened 😅) :)

After taking a look:

  • The logic in the two functions should be deduplicated
  • There should be a docs entry for the new functions in the NixOS manual (there's a sections on formats)

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation labels Jan 8, 2025
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 9, 2025
@Stunkymonkey Stunkymonkey removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 9, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Jan 9, 2025
@Stunkymonkey
Copy link
Contributor Author

@infinisil thanks for the feedback.

the two functions were placed to give a choice which one is preferred.

Additionally I fixed the merge-conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants