search

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

exports doesn't work for node 5 in node-json-minify #32

Closed fuzing closed 8 years ago

fuzing commented 8 years ago

Hi Kyle:

Could you change the 'exports' to 'module.exports' so that it continues to work correctly in node-5.

Cheers,

PB

getify commented 8 years ago

I would happily accept a PR.

fuzing commented 8 years ago

Hi Kyle:

I would have done that, except the package pointed to at github from the ‘node-minify-json’ (at npmjs.com) doesn’t appear to be the same…….

Is there another github link I should be using?

Cheers, and sorry for the confusion,

Peter B.

On Mon, Nov 9, 2015 at 2:15 PM, Kyle Simpson notifications@github.com wrote:

I would happily accept a PR.

— Reply to this email directly or view it on GitHub https://github.com/getify/JSON.minify/issues/32#issuecomment-155196784.

fuzing commented 8 years ago

Sorry - I mean ‘node-json-minify’ (not ‘node-minify-json’)

On Mon, Nov 9, 2015 at 2:23 PM, Peter Beresford peter@fuzing.com wrote:

Hi Kyle:

I would have done that, except the package pointed to at github from the ‘node-minify-json’ (at npmjs.com) doesn’t appear to be the same…….

Is there another github link I should be using?

Cheers, and sorry for the confusion,

Peter B.

On Mon, Nov 9, 2015 at 2:15 PM, Kyle Simpson notifications@github.com wrote:

I would happily accept a PR.

— Reply to this email directly or view it on GitHub https://github.com/getify/JSON.minify/issues/32#issuecomment-155196784.

getify commented 8 years ago

@fuzing -- awhile back, this repo was reorganized to have each lang/env port on its own branch. you might want to look at the "node" branch:

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

pradyunsg commented 8 years ago

@fuzing Please confirm this is now fixed.

fuzing commented 8 years ago

I have no idea .... Given that it was a one line fix and after putting in a PR you came back with a 'no way', I pursued a completely different solution. Single line fixes should be easy....... Not impeded

On Thursday, November 12, 2015, Pradyun notifications@github.com wrote:

@fuzing https://github.com/fuzing Please confirm this is now fixed.

— Reply to this email directly or view it on GitHub https://github.com/getify/JSON.minify/issues/32#issuecomment-156026017.

fuzing commented 8 years ago

I just went to the node repo and it does appear that the single line in question has been changed. Next time it would be great if, when the author (Kyle) suggests that I submit a PR, you grant it. This very simple one line fix could have been implemented in 30 seconds instead of the days it took.

On Thursday, November 12, 2015, Peter Beresford peter@fuzing.com wrote:

I have no idea .... Given that it was a one line fix and after putting in a PR you came back with a 'no way', I pursued a completely different solution. Single line fixes should be easy....... Not impeded

On Thursday, November 12, 2015, Pradyun <notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

@fuzing https://github.com/fuzing Please confirm this is now fixed.

— Reply to this email directly or view it on GitHub https://github.com/getify/JSON.minify/issues/32#issuecomment-156026017.

getify commented 8 years ago

@fuzing -- the question wasn't about the fix, but about where the fix should be implemented within the organization of the project. When I suggested you look at the node fork, it already had the fix you were asking for. It just hadn't yet been pushed out as a new npm release with a version bump. That was an oversight, not some attempt at "impeding" your progress.

You might also have been able to see the status of that branch (already with the code you wanted) and asked about bumping the version, or better yet, PR'ing it, so that it went out to npm. That might have gotten it out a little quicker than the "days" it took (which was < 3, btw).

pradyunsg commented 8 years ago

@fuzing

It was the fact that the PR was targeted at the wrong branch in the repository that was the issue with the PR.

As @getify menitioned, the fix was already a part of the repository when the issue was opened, just not published. The project repository had recently been re-organized, which broke the npm module. All relevant code has been moved to a different branch but the npm repository was not updated for the change. So, in fact this issue should have been fixed by updating the npm repository. The fact that the re-organization wasn't documented well (or at all?) did not help the situation either. See #23 for more context about the re-organization.

I should have made these more clear in my comment on the relavant PR. So, I should apologize for being overly terse in the relavant PR comment.

Sorry! :disappointed:

Note to self: Be less terse with PR comments. Make sure the other person knows enough context to justify your action.

(Editted)

pradyunsg commented 8 years ago

As OP, @fuzing could you please confirm that this issue has indeed been fixed?

fuzing commented 8 years ago

Pradyun - thanks for your response…….. Yes it does appear fixed - thank you. Kyle, I didn’t mean to get into a pissing contest with you, so I apologize if I came off a little abruptly. I was just a little annoyed at being bounced around for a single line code change that would have taken 10 seconds to fix …… I’m mindful of productivity. Sorry.

On Thu, Nov 12, 2015 at 8:28 AM, Pradyun notifications@github.com wrote:

As OP, @fuzing https://github.com/fuzing could you please confirm that this issue has indeed been fixed?

— Reply to this email directly or view it on GitHub https://github.com/getify/JSON.minify/issues/32#issuecomment-156138112.

fuzing commented 8 years ago

resolved

getify commented 8 years ago

Cheers, glad things are resolved. Sorry for the slow/miscommunications.

fuzing commented 8 years ago

No worries Kyle - I know you’re busy! Thanks for your response. Cheers, PB

PS. I’m back to using your solution (i.e. the npm variant), having originally just pilfered your code and made the one line change (it’s cleaner this way!). Be well.

On Thu, Nov 12, 2015 at 11:33 AM, Kyle Simpson notifications@github.com wrote:

Cheers, glad things are resolved. Sorry for the slow/miscommunications.

— Reply to this email directly or view it on GitHub https://github.com/getify/JSON.minify/issues/32#issuecomment-156194486.