amazon-ion / ion-python

A Python implementation of Amazon Ion.
https://amazon-ion.github.io/ion-docs/
Apache License 2.0
260 stars 50 forks source link

Support trailing commas in `ion.dump[s]()` #370

Open mavjop opened 3 days ago

mavjop commented 3 days ago

I have code that I want to read in ion data from a file, make a modification, and then write it back out to a file. The file will be source controlled in a version control system.

I want to include trailing commas, e.g.:

{
    foo: "bar",
    baz: {
        quux: [
            "thing1",
            "thing2",
            "thing3",
        ],
    },
}

That is, a comma after the last item in a list, a comma after the last item in a map, etc.

Ion supports this, which is a huge plus in my book (kudos, Ion!).

This results in diffs that are minimized (if you add or remove a line, there aren't additional lines of diff which only contain addition or removal of a comma), version conflicts are cleaner, and it reduces the likelihood of errors when humans may also edit the files.

I don't think there is any support for asking dump or dumps to include trailing commas (I spent a little time looking for one), so I would love if we could add one. I'm most interested in ion.dumps(), but a flag could be added to any other methods that make sense (dump()? Others?).

- Stephen

P.S. For more on why trailing commas are a good idea, please see On the virtues of the trailing comma.

rmarrowstone commented 2 days ago

Some notes:

mavjop commented 2 days ago

It ... does do pretty printing for me? I'm calling it as:

    ion.dumps(content, binary=False, omit_version_marker=True, indent=u'   ')

I assume the indent=u' ' directs it to do multi-line pretty printing, with a (in our case) three space indent.

I was thinking it could take another optional argument such as trailing_commas=True, defaulted to False for backwards compatibility. I saw that all of the library seemed to be written in C. It's been a few years since I've used C in anger, but I did consider having a look at how one might add code. :)

Is there a concern about maintaining feature parity with ion libraries for other languages?

I agree that it would be relatively easy to implement in pure Python, but potentially a little more tricky in C, though perhaps not horrendously so.

rmarrowstone commented 1 day ago

oh. you are correct. corrected comment.

I was thinking it could take another optional argument such as trailing_commas=True, defaulted to False for backwards compatibility.

That sounds good to me.

Is there a concern about maintaining feature parity with ion libraries for other languages?

I'd say no. But I will check with the other maintainers.

popematt commented 1 day ago

Is there a concern about maintaining feature parity with ion libraries for other languages?

I'd say no. But I will check with the other maintainers.

API feature parity with other libraries is not a concern. The only real cross-library "parity" concern is that they can all read data written by other implementations. Since trailing commas are allowed by the spec, if any libraries cannot read data with trailing commas, it is a defect in that library and should not block the proposed change here.

TLDR; No concerns—do it!