Closed natewalck closed 8 years ago
I generally do not want external dependencies in my projects if they can be avoided, though in the case at least the external dependency is optional. Convince me this is worth it! It already added non-trivial additional complexity to support json; yaml ups this even more. And there are more text formats for structured data... where does it end? INI? That other JSON/YAML-like format someone pointed to on MacAdmins Slack recently?
Additionally: "installing" and using munki-pkg can be done by a non-privileged user; using pip (is that installed by default on all supported versions of OS X) to install PyYAML requires sudo privs.
If pip isn't available, people will need to be instructed to use easy_install to either install pip, or to directly install PyYAML.
UCL: https://github.com/vstakhov/libucl <--- which you pointed to on Slack RSON and RSONlite: https://code.google.com/archive/p/rson/ TOML: https://npf.io/2014/08/intro-to-toml/ AXON: http://intellimath.bitbucket.org/blog/posts/axon-as-better-version-of-yaml.html
I find this a bit concerning about PyYAML: http://nedbatchelder.com/blog/201302/war_is_peace.html
All fair points.
While I am using an external dependency, do you prefer any of those mentioned to YAML? I can change it to use safe_load or look at doing UCL. My concern with the other formats is they are either not very popular or too new (in general).
No, I don't think support should be added for those other formats. Just concerned that six months ago you were eager to add JSON support. Today you want to add YAML support (which increases the support complexity due to the external dependency). Will you tire of YAML in six more months and want to add TOML or UCL?
Not at all. Mike was looking at data type compatibility and trying to find a format that supported multiline strings and standardize on it. The biggest issue with json is lack of multiline string and NSDate support. It is pretty easy to handle the NSDate case with a simple translation layer, but multiline strings are a deal breaker.
It's also amusing (to me at least) that if munkipkg used FoundationPlist (or equivalent) instead of plistlib, this would be a perfectly valid plist format:
{
"distribution_style" = 1;
identifier = "com.github.munki.pkg.SuppressSetupAssistant";
"install_location" = "/";
name = "SuppressSetupAssistant.pkg";
ownership = recommended;
"postinstall_action" = none;
"suppress_bundle_relocation" = 1;
version = "1.0";
}
...which looks like Yet Another Variant of JSON/YAML/TOML/etc.
No need (yet) for NSDates or NSData in munkipkg's build-info, so that bullet does not yet need to be dodged.
"a format that supported multiline strings" --- like, hmm, XML plists! I, too, dislike JSON's (non-)support of multiline strings.
For sure. In the context of munki-pkg, it is not needed (yet if ever).
This was a quick hack to see how it would look and if it was acceptable or not. YAML makes diffs way more readable than plists, so if it worked out here, it might be useful elsewhere to review changes.
I'll accept this PR with two caveats:
1) I will document that the preferred data format for build-info is XML-plist, and less testing is done with the alternate formats. People who decide to use the alternate formats are expected to understand the finer points of using them. (IOW, don't ask for help formatting your JSON or your YAML)
2) I will not promise that all future features added to munkipkg will function properly with non-XML-plist format build-info files. This is an escape clause for me if I do need to use dates or data items for some future features. (or something else we have not yet considered)
My 2c:
Given plists that are as tiny as the plists used by munki-pkg, and:
<string>
tag, and.. I wouldn't think diffing plists in this context is that bad.
In the case of munki-pkg, you are correct. It is already a small amount of data, but it is precisely the same ratio of taglength:text. The ratio of taglength:text makes it equally annoying for diffs in my view.
For example, when the tags for the string are longer than the string itself, it gets absurd:
<string>Firefox</string>
Sure it is a quibble, but I think improved readability for diffs is a huge win for teams of 2+.
Perhaps what you are looking for is a different tool for reviewing diffs which shows changes within lines?
On Friday, January 29, 2016, Nate Walck notifications@github.com wrote:
In the case of munki-pkg, you are correct. It is already a small amount of data, but it is precisely the same ratio of taglength:text. The ratio of taglength:text makes it equally annoying for diffs in my view.
For example, when the tags for the string are longer than the string itself, it gets absurd:
Firefox Sure it is a quibble, but I think improved readability for diffs is a huge win for teams of 2+.
— Reply to this email directly or view it on GitHub https://github.com/munki/munki-pkg/pull/14#issuecomment-176987411.
Samuel Keeley
BTW, munkipkg is a pretty trivial tool and Mike loves Ruby. Why don't youse guys make a Ruby clone? Then you can pervert it to your specific needs. Seems like maybe an hour's work at most for a competent Rubyist...
@keeleysam No such tool exists that would work for our needs ;)
@gregneagle True. This was more proof of concept than anything. I'll go convert munki to YAML now, how hard could it be? jeremy clarkson voice :)~
@natewalck So do you plan to reply or react to this comment, or did you miss it in all the excitement?
https://github.com/munki/munki-pkg/pull/14#issuecomment-176985756
@gregneagle Oh, I had typed up a response and forgot to post it ;)
Those two caveats are completely fair. Plists as default on OS X make sense.
I'll make a task to get #1 done on Monday (or this weekend if I get a break).
Yay.
Sent from my iPhone
On Jan 29, 2016, at 4:21 PM, Nate Walck notifications@github.com wrote:
I'll make a task to get #1 done on Monday (or this weekend if I get a break).
— Reply to this email directly or view it on GitHub.
@erikng Don't get too excited -- I'm not sure that means what GitHub thinks it does.
On Jan 29, 2016, at 4:30 PM, Greg Neagle notifications@github.com wrote:
@erikng Don't get too excited -- I'm not sure that means what GitHub thinks it does.
1 Pick up milk
2 Drop off mail
— Reply to this email directly or view it on GitHub.
Oops, I mean the first item on @gregneagle's caveat list.
Here is an example of what yaml looks like for SuppressSetupAssistant:
Confirmed this worked for --create and --import.