PR#58: Preserve field ordering in dumps/dumps_all - modulemd

modulemd

#58 Preserve field ordering in dumps/dumps_all

Merged 6 years ago by ralph. Opened 6 years ago by crobinso.

crobinso/modulemd dump-ordered into master

Preserve modulemd field ordering in dumps/dumps_all

Cole Robinson • 6 years ago

362ffeb

Add internal method to dump into an OrderedDict

Cole Robinson • 6 years ago

1a7b656

Use yaml.safe_dump_all in dumps_all()

Cole Robinson • 6 years ago

f47a938

modulemd/__init__.py

file modified

+63 -23

		`@@ -40,6 +40,7 @@`
		`mmd.dump("out.yaml")`
		`"""`

		`+ from collections import OrderedDict`
		`import sys`
		`import datetime`
		`import dateutil.parser`
		`@@ -60,6 +61,22 @@`

		`supported_mdversions = ( 1, )`

		`+ # From https://stackoverflow.com/a/16782282`
		`+ # Enable yaml handling of OrderedDict, rather than serialize`
		`+ # dict values alphabetically`
		`+ def _represent_ordereddict(dumper, data):`
		`+ value = []`
		`+`
		`+ for item_key, item_value in data.items():`
		`+ node_key = dumper.represent_data(item_key)`
		`+ node_value = dumper.represent_data(item_value)`
		`+`
		`+ value.append((node_key, node_value))`
		`+`
		`+ return yaml.nodes.MappingNode(u'tag:yaml.org,2002:map', value)`
		`+ yaml.representer.SafeRepresenter.add_representer(OrderedDict,`
		`+ _represent_ordereddict)`
		`+`
		`def load_all(f):`
		`"""Loads a metadata file containing multiple modulemd documents`
		`into a list of ModuleMetadata instances.`
		`@@ -98,7 +115,8 @@`

		`:param list l: List of ModuleMetadata instances`
		`"""`
		`- return yaml.dump_all([x.dumpd() for x in l], explicit_start=True)`
		`+ return yaml.safe_dump_all([x._dumpd_ordered() for x in l],`
		`+ explicit_start=True)`

		`class ModuleMetadata(object):`
		`"""Class representing the whole module."""`
		`@@ -341,17 +359,17 @@`
		`with open(f, "w") as outfile:`
		`outfile.write(data)`

		`- def dumpd(self):`
		`- """Dumps the metadata into a dictionary.`
		`+ def _dumpd_ordered(self):`
		`+ """Dumps the metadata into a OrderedDict.`

		`- :rtype: dict`
		`+ :rtype: collections.OrderedDict`
		`"""`
		`- doc = dict()`
		`+ doc = OrderedDict()`
		`# header`
		`doc["document"] = "modulemd"`
		`doc["version"] = self.mdversion`
		`# data`
		`- d = dict()`
		`+ d = OrderedDict()`
		`if self.name:`
		`d["name"] = self.name`
		`if self.stream:`
		`@@ -366,18 +384,18 @@`
		`d["description"] = self.description`
		`if self.eol:`
		`d["eol"] = str(self.eol)`
		`- d["license"] = dict()`
		`+ d["license"] = OrderedDict()`
		`d["license"]["module"] = sorted(list(self.module_licenses))`
		`if self.content_licenses:`
		`d["license"]["content"] = sorted(list(self.content_licenses))`
		`if self.buildrequires or self.requires:`
		`- d["dependencies"] = dict()`
		`+ d["dependencies"] = OrderedDict()`
		`if self.buildrequires:`
		`d["dependencies"]["buildrequires"] = self.buildrequires`
		`if self.requires:`
		`d["dependencies"]["requires"] = self.requires`
		`if self.community or self.documentation or self.tracker:`
		`- d["references"] = dict()`
		`+ d["references"] = OrderedDict()`
		`if self.community:`
		`d["references"]["community"] = self.community`
		`if self.documentation:`
		`@@ -387,39 +405,39 @@`
		`if self.xmd:`
		`d["xmd"] = self.xmd`
		`if self.profiles:`
		`- d["profiles"] = dict()`
		`+ d["profiles"] = OrderedDict()`
		`for profile in self.profiles.keys():`
		`if self.profiles[profile].description:`
		`if profile not in d["profiles"]:`
		`- d["profiles"][profile] = dict()`
		`+ d["profiles"][profile] = OrderedDict()`
		`d["profiles"][profile]["description"] = \`
		`str(self.profiles[profile].description)`
		`if self.profiles[profile].rpms:`
		`if profile not in d["profiles"]:`
		`- d["profiles"][profile] = dict()`
		`+ d["profiles"][profile] = OrderedDict()`
		`d["profiles"][profile]["rpms"] = \`
		`sorted(list(self.profiles[profile].rpms))`
		`if self.api:`
		`- d["api"] = dict()`
		`+ d["api"] = OrderedDict()`
		`if self.api.rpms:`
		`d["api"]["rpms"] = sorted(list(self.api.rpms))`
		`if self.filter:`
		`- d["filter"] = dict()`
		`+ d["filter"] = OrderedDict()`
		`if self.filter.rpms:`
		`d["filter"]["rpms"] = sorted(list(self.filter.rpms))`
		`if self.buildopts:`
		`- d["buildopts"] = dict()`
		`+ d["buildopts"] = OrderedDict()`
		`if self.buildopts.rpms:`
		`- d["buildopts"]["rpms"] = dict()`
		`+ d["buildopts"]["rpms"] = OrderedDict()`
		`if self.buildopts.rpms.macros:`
		`d["buildopts"]["rpms"]["macros"] = \`
		`self.buildopts.rpms.macros`
		`if self.components:`
		`- d["components"] = dict()`
		`+ d["components"] = OrderedDict()`
		`if self.components.rpms:`
		`- d["components"]["rpms"] = dict()`
		`+ d["components"]["rpms"] = OrderedDict()`
		`for p in self.components.rpms.values():`
		`- extra = dict()`
		`+ extra = OrderedDict()`
		`extra["rationale"] = p.rationale`
		`if p.buildorder:`
		`extra["buildorder"] = p.buildorder`
		`@@ -435,9 +453,9 @@`
		`extra["multilib"] = sorted(list(p.multilib))`
		`d["components"]["rpms"][p.name] = extra`
		`if self.components.modules:`
		`- d["components"]["modules"] = dict()`
		`+ d["components"]["modules"] = OrderedDict()`
		`for p in self.components.modules.values():`
		`- extra = dict()`
		`+ extra = OrderedDict()`
		`extra["rationale"] = p.rationale`
		`if p.buildorder:`
		`extra["buildorder"] = p.buildorder`
		`@@ -447,18 +465,40 @@`
		`extra["ref"] = p.ref`
		`d["components"]["modules"][p.name] = extra`
		`if self.artifacts:`
		`- d["artifacts"] = dict()`
		`+ d["artifacts"] = OrderedDict()`
		`if self.artifacts.rpms:`
		`d["artifacts"]["rpms"] = sorted(list(self.artifacts.rpms))`
		`doc["data"] = d`
		`return doc`

		`+ def dumpd(self):`
		`+ """Dumps the metadata into a dictionary.`
		`+`
		`+ :rtype: dict`
		`+ """`
		`+ def _convert_ordered(orig, new):`
		`+ """Recurse over a nested OrderedDict, converting each to`
		`+ a dict()`
		`+ """`
		`+ for key, val in orig.items():`
		`+ if not isinstance(val, OrderedDict):`
		`+ new[key] = val`
		`+ continue`
		`+`
		`+ new[key] = dict()`
		`+ _convert_ordered(val, new[key])`
		`+`
		`+ ordered = self._dumpd_ordered()`
		`+ converted = dict()`
		`+ _convert_ordered(ordered, converted)`
		`+ return converted`
		`+`
		`def dumps(self):`
		`"""Dumps the metadata into a string.`

		`:rtype: str`
		`"""`
		`- return yaml.safe_dump(self.dumpd(), default_flow_style=False)`
		`+ return yaml.safe_dump(self._dumpd_ordered(), default_flow_style=False)`

		`@property`
		`def mdversion(self):`

crobinso commented 6 years ago

I noticed that 'fedmod rpm2module' and mbs-build modules.yaml output has alphabetic field ordering, which doesn't match modulemd file conventions. This PR fixes it, by formating the data internally into an OrderedDict, and teaching yaml how to handle it. Public API is unchanged, but maybe _dumpd_ordered could be public, or dumpd grow an ordered= argument or something. Suggestions welcome or just fix it as you see fit

ralph commented 6 years ago

Looks good to me.

@sgallagh, does your team still look after this lib?

Does this change have any relation to the modulemd C implementation you worked on?

sgallagh commented 6 years ago

@ralph No, the libmodulemd implementation has a fixed output format that matches the spec.yaml exactly.

ralph commented 6 years ago

@sgallagh - can you advise? Is libmodulemd in a place where we should start porting tools to it (like fedmod rpm2module and mbs-build)?

Or, should we accept this patch and cut a new python-modulemd release?

sgallagh commented 6 years ago

@ralph: For now, accept this patch. Our libmodulemd is planned to gain a Python wrapper such that it will be a drop-in replacement for python-modulemd, but it's on the back-burner at the moment.

I suppose you could opt to plug into the GObject Introspection Python interface provided by libmodulemd if you wanted, but I am not prepared to suggest that's a requirement at this stage.