jitsu deploy messed up my package.json indentation #208

Closed
ljharb opened this Issue Apr 2, 2012 · 14 comments

4 participants

@ljharb

My package.json file, like the rest of my app, happens to use tab-character indentation. When doing my first jitsu deploy, my package.json file was modified (which is fine), but all of my tabs were converted to pairs of spaces.

Without making any comments about the tabs vs 4 spaces vs any other number of spaces debate, jitsu should either figure out what indentation mechanism i'm using, or at the least, not change whitespace on lines it's not directly adding/modifying.

@ljharb

Another possibility is using a machine-specific setting, that could be set as part of the deploy, simply asking me to choose tabs, or X spaces for indentation.

@jfhbrook

This is basically a dupe of #87 so I'm going to close this issue and move discussion over there.

When jitsu runs, it reads and writes to your package.json for a number of reasons. Internally, what happens is something like this:

var fs = require('fs');

var data = JSON.parse(fs.readFileSync(__dirname + '/config.json').toString());

// do some stuff with data
data.foo = "bar";

fs.writeFileSync(__dirname + '/config.json', JSON.stringify(data, true, 2));

The upshot is that we're able to validate your package.json and help you make it valid if it's not ready to go to our servers. The downshot, of course, is that JSON.stringify isn't all that sophisticated.

We would love it if there was a JSON stringifyer out there that could handle reading, writing and updating new properties in an existing JSON string, and if there were a pull request such that this problem were taken care of in jitsu we would accept.

The project that most likely has the relevant code is nconf. You may also be interested in jsup but I believe it can only modify in-place values.

@jfhbrook jfhbrook closed this Apr 2, 2012
@Marak Marak reopened this Apr 5, 2012
@Marak

@jesusabdullah is correct in what he is saying about formatting, but I believe we could make a simple change that might help...

If we exposed the spacing argument to JSON.stringify as configurable in nconf, we could then add a setting to jitsu which enabled users to specify a numeric value for amount of indentation spaces. It also occurs to me that \t might also work...maybe not.

see:

https://github.com/flatiron/nconf/blob/master/lib/nconf/stores/file.js#L54
https://github.com/flatiron/nconf/blob/master/lib/nconf/stores/file.js#L68

@ljharb - Do you think that would help at all?

@ljharb

I like that idea - maybe instead of a numeric value, we could just specify a string representing "one level of indentation" whether that was N spaces or a tab?

@Marak

I think both might actually work. Either way, we should probably just expose the JSON.stringify arguments in nconf exactly as they are defined.

This looks like a good reference: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/JSON/stringify

This isn't a high priority for us right now, but if you are able to open a pull request to nconf, I'd be happy to help merge everything in and update jitsu.

:-)

@ljharb

I can pull request to nconf - should I just make the change in formats.js, or also in file.js? If in file.js, where should the space argument come from?

@Marak

Should be in formats.js. Upon further review, this looks like this it might require a minor refactor to nconf.

See what you can do. If anything, open up a github issue on nconf requesting to add this new feature as well as explaining it and I'll make sure someone reviews it.

@ljharb

Ping @Marak - the nconf changes have been merged in

@Marak

Cool! I will add this feature soon.

I've got a new version of jitsu I'm working on locally. I'll make sure this gets in.

@Marak

Waiting on npm permissions to publish nconf. Will add soon.

@ljharb

Any luck on npm permissions?

@indexzero
nodejitsu member

@ljharb @Marak nconf is published with this change and jitsu@HEAD now includes that version

@yawnt yawnt referenced this issue Sep 18, 2012
Merged

Json indentation #316

@jfhbrook

After merging yawnt's pull request, the particular issue of indentation detection/application is considered fixed.

As noted elsewhere, this doesn't completely address #87, as the scope of that issue is greater (also preserving comma-first, bracket alignment and other more exotic layout preferences.

@jfhbrook jfhbrook closed this Sep 19, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment