getify / JSON.minify

Simple minifier for JSON to remove comments and whitespace
http://web.archive.org/web/20100629021329/http://blog.getify.com/2010/06/json-comments/
410 stars 102 forks source link

Put different language implementations on different branches (Will break all other PRs) #23

Closed pradyunsg closed 8 years ago

pradyunsg commented 9 years ago

I had a small suggestion, that you put the implementations in separate branches, one for each language. This way, language-specific packaging can be easily done, in a single repository.

Just what I think.... :smile:

PS: I will happily make a PR for solving #21 if this is done.



getify commented 9 years ago

This is a good idea to improve organization.

But do you think that just separate folders could accomplish the same thing... that is, one top-level directory for each language, where everything (including build stuff, tests, etc) needed is per-folder?

pradyunsg commented 9 years ago

I considered both, in fact I have 2 clones, one for each arrangement, on my system right now, and concluded that this is a better arrangement.

I think this arrangement makes distribution directly off the repository easier as many packaging/distribution tools support installing/fetching from repository branches, but support for a sub-directory of a repository is less likely or at least a bit more tricky, especially without a file for every language in the root directory which, in a way, defeats the purpose of the separation/organization.

Plus, then it is also possible to have language-specific things that would work with branches (Like CI).

BTW: I forgot to mention, I have arranged my fork like proposed (with some minor changes possibly, like using an independent base branch (instead of forked from master) etc). This is approximately how it would look like... After adding the Python distribution stuff and QUnit tests. :)

Lastly, Happy New Year! :tada:

pradyunsg commented 9 years ago

Ping!

getify commented 9 years ago

Appreciate the ping. I haven't gotten around to this because there's still many fires above it on my list. But I would happily consider a PR. ;-)

pradyunsg commented 9 years ago

Well, I would have made a PR, but I can't think of a way to have PRs for so many branches...

BTW, Good great going with YDKJS. :+1:

getify commented 9 years ago

Seems like it might be a PR for the main repo, plus a PR for each new branch. Seems a bit unwieldy, but might actually be good to consider each one separately to make sure details are proper.

pradyunsg commented 9 years ago

Well, how do I propose a new branch with a PR?

getify commented 9 years ago

I may be mistaken, but I believe if you create a separate branch in your fork, and submit a PR, you can target a branch on the main repo besides master. May (likely) be that we need to create those empty branches on this repo first. If so, first compiling a list of what main repo branches (by name) should be created would be quite helpful. :)

pradyunsg commented 9 years ago

Well, looks like making new branches on this repository is the way to go. Here's my proposed list of branch-names:

(Editted): "empty" -> "new"

getify commented 9 years ago

what will go in base different from master? moreover, what will be in master?

also, we'll need node separate from javascript, right?

pradyunsg commented 9 years ago

what will go in base different from master?

I think of base as being a "base" for all the ports (to base their work off) [the name came organically].

moreover, what will be in master?

Just some documentation (about the universal algorithm), generic stuff (Porting instructions) etc.

also, we'll need node separate from javascript, right?

I'm not so sure of that, after-all whatever's there in JS should be packaged as well, right?

getify commented 9 years ago

It's unclear at this point what base would actually have. If you can be more specific, that might motivate it. At the moment, my inclination would be to put whatever you're thinking for base into the documentation in master as you suggest.

No, right now the node version is modularized whereas the straight JS version is stand-alone (more suitable for copy-n-paste inclusion in other libs). I think I would want to preserve that.

pradyunsg commented 9 years ago

My motivation behind having a base: To have a clear point from where someone can start work on a new language port. Also, it will be the point in the history from which every language port will be based. As an addon, having a single point in the history and basing everything off that point will give us a nice history as it won't mix language-specific history. It would need rebasing language branches every time a commit is made to the base, but that should be very rare anyway, as documentation should be in master.

PS: That feels repetitive but I should really be sleeping right now anyway. :stuck_out_tongue: (Editted, Cropped)

pradyunsg commented 9 years ago

On second thought, how exactly is a port to be PRed? I mean, if we need to make a branch on this repository before making a new PR for a language, I don't see that as convenient, for anyone involved. While it seems to be out-weighed by the advantages of the re-organization, it's an interesting problem. Any ideas about this?

getify commented 9 years ago

Instinct reaction: I think if someone's going to contribute a whole new port of the lib, having them show up completely unannounced with a PR is less ideal than first asking in an issue, understanding design philosophy, discussion options, etc. In this case, we can then create a branch if it seems appropriate for such a PR.

pradyunsg commented 9 years ago

I agree, but then there's #24. (As a counter-argument for the sake of it)

getify commented 9 years ago

That sort of thing wouldn't have happened if the reorg had already been in place. Would it have prevented the contribution entirely? I doubt it. Is it slightly more effort on a contributor's part? Yep.

pradyunsg commented 9 years ago

Yes, I was gonna say that...

Well then, looks like all that's left here is you making the required branches here and me sending the required PRs. Then the discussion(s) should move to those PRs.

getify commented 9 years ago

I will try to find sometime this weekend to sit down and make the branches. :)

getify commented 9 years ago

OK, first step done... I've created the "base" branch, here:

https://github.com/getify/JSON.minify/tree/base

  1. Please have a look at that content, see if you think it's appropriate and correct as a starting point.
  2. Once we agree it's good to go, I will start making the language branches (I'll handle JS, node, and PHP, since I wrote those originally) by branching off "base".
  3. Once we're happy with how that's gone, then I'll create the other language branches, and you can help set them up.
  4. Finally, once there's agreement that all the language branches are in order, we can clean up master by removing all language files and introducing updates to the documentation about porting, etc.

Sound good?

pradyunsg commented 9 years ago

Yep! Sounds good.

getify commented 9 years ago

OK, so (1) is now done on the above list. (2) is next. I'll get to that ASAP. Sorry I'm so slow, juggling lots of things! :)

getify commented 9 years ago

Is it fine to call the various language ports in this repository as "official"?

yep

pradyunsg commented 9 years ago

Worth a mention: Using labels to categorize issues and PRs as common, algorithm, documentation, language etc.

pradyunsg commented 9 years ago

Added a good looking task-list to the issue for branch-wise tracking and linking purposes.

You will make a PR for every language, right? (Same-repo PRs could from node-dev to node maybe..)

pradyunsg commented 9 years ago

Ping v2.0! Mind giving me push access to this repository?

getify commented 9 years ago

@pradyunsg

Thanks again for staying on top of this. Update:

  1. I've created and pushed branches to this repo for all the language ports as listed in the checklist above
  2. I've set up the "javascript", "node", and "php" branches with code and tests, and moved those respective files out of master.
  3. I've made you a contributor now.

I would certainly appreciate eyeballs and help making sure that stuff is correct, and also help doing the "python", "java", and "objective-c" branch files.

Please check how I did the "test suite" stuff in each of the branches so far. I'd like to keep something close'ish if possible for the other ports, though not strictly required ofc.

Hope that helps get things moving now. :)

pradyunsg commented 9 years ago

Things did move.. :smile:

pradyunsg commented 9 years ago

thought... Should the PORTING.md be a part of the Wiki?

getify commented 9 years ago

I don't have a strong feeling either way. I don't know that the wiki adds any value that the base repo branch doesn't already have, but if there's some reason to move that way, I wouldn't mind.

pradyunsg commented 9 years ago

Ok!

pradyunsg commented 8 years ago

I'm putting the Java port on the side-line as "will fix later" and writing relavant documentation for master branch.

getify commented 8 years ago

LGTM.

pradyunsg commented 8 years ago

Just merged java PR. Completely done.