Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selective ignore using comments (feature request) #384

Closed
strille opened this issue Jan 14, 2014 · 15 comments
Closed

Selective ignore using comments (feature request) #384

strille opened this issue Jan 14, 2014 · 15 comments

Comments

@strille
Copy link

strille commented Jan 14, 2014

It would be nice to be able to have sections of code which are ignored by js-beautify (similar to the control comments available in jslint).

For instance, it would be helpful in cases where you have definitions of static data which you want to format in a specific way for increased readability and compactness.

Something along the lines of:

/*ignore js-beautify start*/
var data = [{label: "foo", value: 1}, 
            {label: "bar", value: 2},
            {label: "baz", value: 3}];
/*ignore js-beautify end*/ 

Thoughts?

@evocateur
Copy link
Contributor

This is probably best achieved by parsing the AST, �à la #200, although not strictly blocked by that feature. We welcome pull requests.

@QuentinRoy
Copy link

I would love to see that too.

@matiu
Copy link

matiu commented Nov 25, 2014

+1

@mansona
Copy link

mansona commented Dec 9, 2014

The alternative would be that you could omit a file from beautification using a comment at the top, is this currently possible or would it be part of this feature request?

@bitwiseman
Copy link
Member

That would be part of this feature.

@mansona
Copy link

mansona commented Dec 29, 2014

@bitwiseman thanks for the clarification 👍

@bitwiseman bitwiseman added this to the Future milestone Jan 29, 2015
@DVLP
Copy link

DVLP commented May 22, 2015

Why
/ignore js-beautify start/
/ignore js-beautify end/

not:
/ignore js-beautify:start/
/ignore js-beautify:end/

?

@strille
Copy link
Author

strille commented May 29, 2015

I've implemented a first version of this feature, which can be tested here:
https://github.com/strille/js-beautify

Any of the following comments will work:

// 1. starting with the word "ignore", like jslint
/*ignore js-beautify start*/
var data = [{label: "foo", value: 1}, 
            {label: "bar", value: 2},
            {label: "baz", value: 3}];
/*ignore js-beautify end*/ 

// 2. starting with the word "js-beautify" using  a colon, like jshint:
/*js-beautify ignore:start*/
var data = [{label: "foo", value: 1}, 
            {label: "bar", value: 2},
            {label: "baz", value: 3}];
/*js-beautify ignore:end*/ 

// 3. starting with the word "ignore" and using a colon instead of a space:
/*ignore js-beautify:start*/
var data = [{label: "foo", value: 1}, 
            {label: "bar", value: 2},
            {label: "baz", value: 3}];
/*ignore js-beautify:end*/ 

// 4. starting with the word "js-beautify" and using a space instead of colon:
/*js-beautify ignore start*/
var data = [{label: "foo", value: 1}, 
            {label: "bar", value: 2},
            {label: "baz", value: 3}];
/*js-beautify ignore end*/ 

The feature is implemented rather naively, but it seems to work quite well (although I have not tested extensively):

  1. Before we beautify the code we first scan and store information about all the ignore blocks
  2. Then the beautifier runs through all the code
  3. Lastly we restore the beautified code between all the ignore comments to the original form.

The comments themselves get beautified, and if that results in them being indented, then all the lines between the start and end comments get indented as well.

@bitwiseman
Copy link
Member

Wow! Nice start. For clarity and simplicty, let's pick one style and stick to it.
I'd suggest using jshint as the model: http://jshint.com/docs/

// Code here will be linted with JSHint.
/* jshint ignore:start */
// Code here will be ignored by JSHint.
/* jshint ignore:end */

Do you consider the indenting a feature or a bug?

Can I suggest a simpler solution?
Block comments are printed as raw output after the first line:
https://github.com/strille/js-beautify/blob/master/js/lib/beautify.js#L1271

So, if you want to you could add code right here: https://github.com/strille/js-beautify/blob/master/js/lib/beautify.js#L1756

That could would check the contents of the comment (which you already do), and if it is an ignore section, just naively add raw lines to the current comment token until it finds the the close section. This would result in less overhead and extremely consistent behavior for this feature.

It would also mean that unmatched braces/parens would cause problems, but that is a bug that could be addressed later. Make sense?

function() {
   var a = {
        /* beautify ignore:start*/
        b: 0
    };
    /* beautify ignore:end*/
    // Uh-oh! Beautifier doesn't know that the object literal was closed!
    }

@bitwiseman bitwiseman modified the milestones: v1.6.0, Future May 30, 2015
@strille
Copy link
Author

strille commented May 30, 2015

I agree that picking one comment style is probably best for simplicity, and I have no problem with the jshint style.

The indentation is actually a feature. I felt that the "whole" ignore block (start comment, contents and end comment) should indent as a result of the start comment changing indentation (so that the whole "block" stays intact). But it kind of goes against the feature a bit and could cause confusion, so I'm not 100% sure it's a good idea.

In fact, if you define a string literal over multiple lines by using a backslash (which isn't recommended, but that's another story), you definitely don't want to have whitespace added to the beginning of your lines since that would change the string value:

var text = 'This is a sentence \
split up over multiple lines \
for increased readability';

Your suggested implementation definitely feels more like the correct way of doing it rather than beautifying the whole thing and then restoring certain sections. Like you pointed out, it will "throw off" the beautifier a bit though since you could start and end the ignore blocks at any time, just like in your example.

@bitwiseman
Copy link
Member

So... Would you like it implement my suggestion? If not, that's okay too.
Either way, this needs to be ported to python and tests added for it.

@strille
Copy link
Author

strille commented Jun 1, 2015

I might give that a crack, to see how that solution turns out.

@alexdreptu
Copy link

There's some trouble with React's JSX. The code gets indented weirdly. Some option to ignore parts of the code would be nice.

@bitwiseman
Copy link
Member

@alectic - Agreed, there's a lot of places where this would be good.
That said, please look at the existing JSX and E4X issues. If your issue isn't already filed, please file it.

@strille
Copy link
Author

strille commented Jun 12, 2015

A bit of an update: I haven't had time to look at the alternative implementation suggested by @bitwiseman, and I'm not sure I will in the hear future :-/

If anyone wants to give it a crack, feel free to go ahead.

bitwiseman added a commit that referenced this issue Jun 15, 2015
// Text below will remain unchanged
/* beautify preserve:start */
{asdklgh;y;;{}dd2d}
/* beautify preserve:end */

NOTE: this is not quiet ignore - still tracks tokens.
Non-javascript may not work.

Related #384
@bitwiseman bitwiseman modified the milestones: v1.6.0, 1.5.7 Jun 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants