TypeStrong / grunt-ts

A grunt task to manage your complete typescript development to production workflow
https://www.npmjs.com/package/grunt-ts
MIT License
330 stars 121 forks source link

Tsconfig Pretty Json #376

Closed RyanThomas73 closed 7 years ago

RyanThomas73 commented 8 years ago

Would you accept a pull request to take the prettyJSON logic from atom-typescript as well as the use of detect-indent and detect-newline to maintain the formatting of tsconfig.json.

https://github.com/TypeStrong/atom-typescript/blob/master/lib/main/tsconfig/tsconfig.ts

...

import detectIndent = require('detect-indent');
import detectNewline = require('detect-newline');

...

export function prettyJSON(object: any, indent: string | number = 4, newLine: string = os.EOL): string {
    var cache = [];
    var value = JSON.stringify(
        object,
        // fixup circular reference
        function(key, value) {
            if (typeof value === 'object' && value !== null) {
                if (cache.indexOf(value) !== -1) {
                    // Circular reference found, discard key
                    return;
                }
                // Store value in our collection
                cache.push(value);
            }
            return value;
        },
        indent
    );
    value = value.replace(/(?:\r\n|\r|\n)/g, newLine) + newLine;
    cache = null;
    return value;
}

...

if (projectSpec.filesGlob) { // for filesGlob we keep the files in sync
    var prettyJSONProjectSpec = prettyJSON(projectSpec, detectIndent(projectFileTextContent).indent, detectNewline(projectFileTextContent));

    if (prettyJSONProjectSpec !== projectFileTextContent && projectSpec.atom.rewriteTsconfig) {
        fs.writeFileSync(projectFile, prettyJSONProjectSpec);
    }
}
nycdotnet commented 8 years ago

Hi there. If it comes with some tests, positive and negative, sure.

RyanThomas73 commented 8 years ago

I've added the prettyJSON function to the tsconfig.ts module. I need to add the tests.

Any suggestions on what tests you would like me to add? So far I can think of 3 positive tests - preserves tabs, preserves spaces, and preserves newlines. Drawing a blank on what else to add tests for.

Any suggestions on how you want me to add the tests?

I took a look at the existing tests and one option I saw was to add a command line assertion function that loads an input tsconfig, calls either the prettyJSON function or the saveTsConfigSync function, and then verifies that the indentation / new line from the source matches the output.

Let me know what your preferences are and I'l get these added and the PR submitted.

nycdotnet commented 8 years ago

The tests that are specified in the Gruntfile are mostly that way because they're trying to test some end to end stuff (and as an accident of history). You probably just need one of those with a command line assertion function. Then some positive and negative tests possibly in the style of the options reaolver tests if you can find a seam. The Nodeunit tests are generally much faster and simpler to understand.

Feel free to submit the PR right away and just say it's WIP. Thanks for contributing!

nycdotnet commented 8 years ago

As I think about it, I am not sure a command line assertion will help you since this feature doesn't map to a tsc fracture. So I think Nodeunit is your best bet.

RyanThomas73 commented 8 years ago

Hmm so the tests 'pass' but.... I had used atom to manually control crlf and lf in the source/expected files and of course git will normalize these. Even if I explicitly turned off autocrlf other contributors will have it on.

One option, I could replace the use of source/expected files with inline strings in the test functions using "\r\n" and "\n" as needed. Thoughts? If you want me to go the inline string route should I inline all of the source/expected files or just the ones testing newline behavior?

nycdotnet commented 8 years ago

Yes in-lining the ones where the line endings are relevant will be the most cross platform way of doing it. I think opening/replacing/rewriting the files prior to each test run to ensure the line endings are as expected would be fine too. That should only be a few lines of code, hopefully. Thanks for doing this PR! I hope this has been interesting and fun for you.

nycdotnet commented 7 years ago

This is released in grunt-ts v6.0.0-beta.16 on npm. Thank you!