search

planetarypy / pvl

Python implementation of PVL (Parameter Value Language)
BSD 3-Clause "New" or "Revised" License
19 stars 19 forks source link

How is this library versioned? #99

Closed jlaura closed 2 years ago

jlaura commented 2 years ago

Seeing #98 closed got me thinking. How exactly is this library being versioned? With the tick from 1.2 to 1.3 (or 1.1 to 1.2, a core dev will have to let me know) using this library to load ISIS data broke. The fix is to add a grammar keyword to the call. This looks a lot like an API breaking change to me that would seem to need a major version tick with some method like semantic versioning were being used.

So, how is this library versioned? (So that I can align my API stability expectations.)

rbeyer commented 2 years ago

The project's changelog (called HISTORY.rst for historic reasons) indicates Semantic Versioning. The decisions for version ticking are at the mercy of the PVL Committers, which is mostly just me these days. As always with a single-person operation, there's jeopardy for mistakes, and maybe one was made.

However, in this case, I don't think there was one. I'll explain, and we'll see if you agree or not.

From the perspective of this Python pvl module, the tick from 1.2 to 1.3 added some features (which qualified as a minor version bump), and fixed some bugs (which qualified as a patch version bump), since the minor is more significant than the patch, that is the version number that got ticked.

Your argument for a major tick would be that pvl's default Omni loader now did not load() some ISIS output, and so from your perspective, this constituted a breaking change, I get that.

The thing is that the change that resulted in not being able to load that file was a "bugfix" from the perspective of pvl. It was correcting behavior in how pvl interpreted a stream of bytes, which it should always have been doing (if I had implemented things correctly from the beginning). It just so happened that the ISIS code which produced that PVL-text file was writing out bad PVL-text (and had always been doing so), and the combination of lax behavior on the part of pvl allowed that particular flavor of PVL-text to load with earlier versions, but it really shouldn't have, because that PVL-text formulation was always bad, and you should have always been explicitly specifying the PVL grammar or the ISIS grammar which would refuse to acknowledge the funny characters after the "END."

The authors of this Python module can't control or manage all of the wild PVL-text producing and consuming behaviors of other software programs, the only thing we can do is focus on following the spec, and being as compliant as we can. If we patch a bug in our code, that's a patch-level tick, and that was the thinking that I applied when I made that decision. Having more developers than just me involved might have opened an argument about whether this change should have qualified for a major tick (I still don't think it does), or have suggested a different development effort which might have resulted in more forgiving behavior, and I welcome active involvement.

The real problem is that "PVL" is truly a terrible data format. There is a reason why it did not catch on, and a reason why there aren't many public-use libraries that support the many and varied "dialects" of PVL. This Python module attempts to silo and provide interfaces for the various separate dialects (which is why there was a way to parse that PVL-text by specifying some extra arguments with the pvl 1.2 code), and also tries its best to provide a "generic" parsing approach (which I call the Omni dialect) that does its best to consume the widest variety of PVL-text. However, there are some points where even the Omni dialect parsing must have a rule to decide how to interpret something. The "Omni" dialect isn't about following a spec, it is about making decisions that attempt to serve the widest possible span of PVL-text. I made a decision about what that should be, and from my perspective, that was a bug fix. If we had more volunteer developers, rather than choosing to go with how dialect A or dialect B would interpret the byte stream, maybe there could be extra logic that might do more comprehensive analysis of the bytes that would still accommodate that PVL-text via the Omni dialect without a user passing an extra argument telling pvl.load() how to deal with it. PRs are definitely accepted.

cmillion commented 2 years ago

I appreciate this explanation of your thinking.

AndrewAnnex commented 2 years ago

I think @rbeyer's explanation is entirely correct and the expectation for semantic versioning.

If the old behavior for the omni solver is desirable, then an ISIS specific loader that can read the malformed ISIS output can be added to the project. This would allow for an easier version pin of PVL for whatever downstream project @jlaura is working on, and provide API stability from the user expectation side. I believe @rbeyer is suggesting this as well.

jlaura commented 2 years ago

@rbeyer I appreciate the explanation as well! Taking the perspective of the project as the primary motivator for identifying a bug fix vs. an API breaking change that warrants a tick in the major version. I think we have a really nice example of this here on the ISIS side as well, where a labelled bug was fixed by a committer. The application of the bug fix that alters the API from the consumers perspective vs. API breaking change from the developer perspective is probably a really nice discussion to continue over here.

Thanks for weighing in!