kimgr / asn1ate

A Python library for translating ASN.1 into other forms.
Other
69 stars 41 forks source link

Sort the processed items to make the output repeatable #30

Closed mmattice closed 8 years ago

mmattice commented 8 years ago

for f in testdata/*.asn ; do comm -3 --nocheck-order <(python asn1ate/pyasn1gen.py $f | grep -v '# Auto-generated' ) <(python asn1ate/pyasn1gen.py $f | grep -v '# Auto-generated') ; done | wc -l

From master, output is 212 lines of difference.

From this branch, output is 0 lines of difference.

Hash ordering makes the output not repeatable so it's difficult to tell if running it on the same input is giving the same output. I don't know enough about directed graphs to know if this breaks, but I do know that the existing code is not depending on any special ordering of the keys in that hash, so sorting them shouldn't matter.

kimgr commented 8 years ago

Thanks! This makes perfect sense to me after going back and re-understanding what I'd done.

As you say, since the order is entirely undefined today, this can't break anything.

I wonder if there should be a switch to omit the # Autogenerated ... header as well?

One nit inline.

mmattice commented 8 years ago

I added the explicit .keys(). I like the idea of removing the header, but it perhaps should be annotated with the time stamp of the source file to give some sort of chain of custody, but that's outside this PR's scope.

kimgr commented 8 years ago

Ouch! I was just about to merge this, but it doesn't work with Python3:

TypeError: unorderable types: ValueAssignment() < ValueAssignment()

The keys in the dict are the actual assignment sema objects, and Python3 appears to be pickier about ordering... I'll see if I can provide a predicate to sorted() using the assignment's reference name.

kimgr commented 8 years ago

Could you try with this and make sure it gets the same effect:

for node in sorted(graph.keys(), key=lambda a: a.reference_name()):
    if node not in lowlinks:
        strongconnect(node)

Thanks,

kimgr commented 8 years ago

I like the idea of removing the header, but it perhaps should be annotated with the time stamp of the source file to give some sort of chain of custody

You mean use the timestamp of the source file rather than the current date? And that way duck the need to suppress it? I like that. The asn1ate version should probably be included as well, it's every bit as influential as the source file.

mmattice commented 8 years ago

Yeah, that's what I meant about the time stamp.

I updated this to use your suggestion with the reference_name attribute. I also verified that running it under py2 against a py3 invocation sorts the same just for giggles.

kimgr commented 8 years ago

Great, that's what I needed to make sure.

I'll collapse this into a single commit and merge it. Many thanks!

kimgr commented 8 years ago

Merged in 8a53d452, thanks!

I'll take a stab at the header as well, thanks for the idea.

kimgr commented 8 years ago

FYI, I just landed a repeatable but source-dependent header format on master. Thanks for the ideas!