first draft of operator_position functionality #735

olsonpm · 2015-07-16T06:00:09Z

Added verification of operator_position option. Throws error if
invalid.
Possible operator_position options are:
1. undefined (use default behavior)
2. 'before_newline'
3. 'after_newline'
4. 'preserve_newline'
5. 'remove_newline'
Moved some code around in handle_operator in order to separate
conflicting logic when handling operators.
Modified allow_wrap_or_preserved_newline to handle operator
preserved newlines as well.
Fixed a few boolean variable assignments within if statements.
Though not always the case, the expression within the if can
be assigned directly to the variable
Added -> built tests
Within the tests, added an 'inputlib.js' file which holds common
input arrays. This was necessary due to the matrices not working
cleanly with default behavior.
It is worth noting that although the python test file was modified,
this is a result of the built test and not actually providing
a python port for this option (yet).

olsonpm · 2015-07-16T06:03:46Z

So instead of figuring out the redundant indentation logic and attempt to condense the tests via matrices, I just made it easy on myself and separated the tests. I added tests for each combination of operator_position and preserve_newlines, meaning there should be 10 new sections of tests total.

Also, as noted in the commit message - I'm expecting the python tests to fail until we verify the javascript functionality. I can write the logic for python afterward.

bitwiseman · 2015-07-20T18:30:56Z

Let me start by saying - great job. Thanks for putting the full effort into this and I approve of everything you mentioned in your description, including not using matrix style for testing.

The size of the change is a little daunting - though I get that the test portions are largely boilerplate.

I'll take a look at the overall structure of the product code changes so that you are unblocked to do the python porting. Then I'll look at the detailed tests to make sure everything is coming out the way it should.

Thanks again!

bitwiseman · 2015-07-20T18:33:29Z

js/lib/beautify.js

+        opt.operator_position = options.operator_position;
+
+        // throws error if invalid value was passed in
+        verifyOperatorPosition(opt.operator_position);


Maybe change this to sanitizeOperatorPosition and have the function return the value to be assigned (replacing undefined with the default).

Can you clarify "replacing undefined with the default"? I'm expecting undefined to be the default value.

change this to sanitizeOperatorPosition and have the function return the value to be assigned

Will do.

olsonpm · 2015-07-20T18:42:03Z

Yeah I thought the testing might have been overkill. It was more my method of development than me testing the end result (start by ensuring all the operators worked properly -> the special colon case -> then a catch all).

Feel free to let me know what you want slimmed down and I'll do it. However I will wait to hear the full product-changes feedback before working on it further.

Finally, this is my first sizable open source contribution - so don't hesitate to critique anything. Normally I just fork and modify for my personal needs.

bitwiseman · 2015-07-20T18:43:37Z

js/lib/beautify.js

+
+    function verifyOperatorPosition(opPosition) {
+        var validPositionValues = [undefined];
+        Object.keys(OPERATOR_POSITION).forEach(function(pos) {


Not sure if this is compatible with older browsers (IE is a pain).

Here's a polyfill to work around it:
http://tokenposts.blogspot.com.au/2012/04/javascript-objectkeys-browser.html

But, you might have to do this instead:

opPosition = opPosition || OPERATOR_POSITION. before_newline; if (!OPERATOR_POSITION[opPosition]) { throw new Error(); } return opPosition;

Ha - I had no idea IE had trouble with that. I'll implement it without the polyfill.

bitwiseman · 2015-07-20T19:38:01Z

Okay, I'm done with my initial pass.
There's the design issue of needing to make the preserve_newlines <--> operator_position interaction consistent, and some code tweaks, but rather minimal.

Thanks again!

olsonpm · 2015-07-21T15:05:11Z

Sounds good. I'll have time this weekend to take a look and make changes.

olsonpm · 2015-07-24T01:05:44Z

Wow - I think I see my misunderstanding. The operator_position shouldn't do anything if preserve_newlines is false. It's only positioning the operators that are placed next to a newline that the user wants preserved. This is why you're mentioning the default behavior 'undefined' should equal 'before_newline'.

I took the option names far too literally. I'll work on this now and have an update tonight hopefully.

Edit: Ugh - looking back at your comment, the section is different from the <before_newline>. So I guess I still need clarification on the default being before_newline since I assume you don't want to change the default behavior for everyone's beautified code upon updating it.

And I apologize for the number of updates to this thread. I'll leave it alone for now.

bitwiseman · 2015-11-10T01:32:27Z

@olsonpm - I've been away. It looks like you've been away too. You wan to take another swing at this?

olsonpm · 2015-11-10T04:26:53Z

Not a problem.

I think our ideas differ on how to handle default behavior (i.e. no option specified)

My understanding is that it should behave as it did before

It seems from your comment above that before_newline should be the default behavior. Note this will be a breaking change.

bitwiseman · 2015-11-10T05:26:05Z

Hm, It's probably an error in understanding on my part.
I thought OPERATOR_POSITION.before_newline is the same as the current behavior. I'll go look again.

olsonpm · 2015-11-10T05:57:36Z

Referring back to your sample code you can see a few subtle differences:

// Test code
var d = 1;
if (a === b 
    && c) {
    d = ((c * everything / something_else)  
            - a) %
        b;
    e 
        += d;

} else if (!(complex && simple) ||
        (emotion && emotion.strong && emotion.name === 'happy')) {
    cryTearsOfJoy(many ||
        anOcean ||
        aRiver);
}

// Output: actual current 
var d = 1;
if (a === b && c) {
    d = ((c * everything / something_else) - a) %
        b;
    e
        += d;

} else if (!(complex && simple) ||
    (emotion && emotion.strong && emotion.name === 'happy')) {
    cryTearsOfJoy(many ||
        anOcean ||
        aRiver);
}

// Output: operator_positioning = before_newline
var d = 1;
if (a === b &&
    c) {
    d = ((c * everything / something_else) -
            a) %
        b;
    e +=
        d;

} else if (!(complex && simple) ||
        (emotion && emotion.strong && emotion.name === 'happy')) {
    cryTearsOfJoy(many ||
        anOcean ||
        aRiver);
}

bitwiseman · 2015-11-10T06:21:12Z

Ah, I see where we're out of sync. In all the complexity, I didn't cover the base case.

  opts.preserve_newlines = false;
  opts.operator_position = undefined;
  bt('var res = a + b - c / d * e % f');   // unchanged
  bt('var res = a + b -\n' +
      '    c / d * e % f',
      'var res = a + b - c / d * e % f');  // newline removed 
  bt('var res = a + b\n' +
      '    - c / d * e % f',
      'var res = a + b - c / d * e % f');  // newline removed 

  opts.operator_position = 'before_newline';
  bt('var res = a + b - c / d * e % f');   // unchanged
  bt('var res = a + b -\n' +
      '    c / d * e % f',
      'var res = a + b - c / d * e % f');  // newline removed 
  bt('var res = a + b\n' +
      '    - c / d * e % f',
      'var res = a + b - c / d * e % f');  // newline removed 

  opts.operator_position = 'after_newline';
  bt('var res = a + b - c / d * e % f');   // unchanged
  bt('var res = a + b -\n' +
      '    c / d * e % f',
      'var res = a + b - c / d * e % f');  // newline removed 
  bt('var res = a + b\n' +
      '    - c / d * e % f',
      'var res = a + b - c / d * e % f');  // newline removed 

  opts.operator_position = 'preserve_newline';
  bt('var res = a + b - c / d * e % f');   // unchanged
  bt('var res = a + b -\n' +
      '    c / d * e % f',
      'var res = a + b - c / d * e % f');  // newline removed 
  bt('var res = a + b\n' +
      '    - c / d * e % f',
      'var res = a + b - c / d * e % f');  // newline removed 

  opts.preserve_newlines = true;
  opts.operator_position = undefined;
  bt('var res = a + b - c / d * e % f');   // unchanged
  bt('var res = a + b -\n' +
      '    c / d * e % f');  // unchanged 

 // NOTE: Below is different from current behavior
  // Current behavior removes the newline, this is a breaking change, but this seems more consistent and correct.
  // However, if we want to be non-breaking, having the newline be removed here is also acceptable.  
  bt('var res = a + b\n' +
      '    - c / d * e % f',
      'var res = a + b -\n' +
      '    c / d * e % f',);  // newline preserved, operator moved to before the newline

  opts.operator_position = 'before_newline';
  bt('var res = a + b - c / d * e % f');   // unchanged
  bt('var res = a + b -\n' +
      '    c / d * e % f');  // unchanged 
  bt('var res = a + b\n' +
      '    - c / d * e % f',
      'var res = a + b -\n' +
      '    c / d * e % f',);  // newline preserved, operator moved to before the newline

  opts.operator_position = 'after_newline';
  bt('var res = a + b - c / d * e % f');   // unchanged
  bt('var res = a + b -\n' +
      '    c / d * e % f',
      'var res = a + b\n' +
      '    - c / d * e % f',);  // newline preserved, operator moved to after the newline
  bt('var res = a + b\n' +
      '    - c / d * e % f');  // unchanged

  opts.operator_position = 'preserve_newline';
  bt('var res = a + b - c / d * e % f');   // unchanged
  bt('var res = a + b -\n' +
      '    c / d * e % f');  // unchanged
  bt('var res = a + b\n' +
      '    - c / d * e % f');  // unchanged

What you have forces newlines after all operators even when there were none. What we want is allow newlines in a specific position for operators and optionally move the newline if the code has it in on the wrong side.

I'm really sorry for not catching this earlier. Totally my bad.

olsonpm · 2015-11-10T14:38:09Z

No problem - I added to the confusion by initially coding it that way. I still have my branch so I can take a look at it this weekend and get back with an up-to-date PR.

Edit: And just to confirm - do we want the breaking change or not? I don't mind coding it either way, I just had assumed we would avoid it.

bitwiseman · 2015-11-12T22:17:38Z

Let's do the breaking change I showed above.

It is different, but I'd argue that it is also more correct. Right now the beautifier punishes people for putting newlines on the wrong side of operators - it looses their intended formatting. The "break" is that is it will try to preserve their formatting. I'm okay with that.

Thanks!

olsonpm · 2015-11-13T05:11:03Z

I'm on board, and definitely agree it's more correct.

olsonpm · 2015-11-16T05:08:48Z

Just an fyi - I didn't find time this weekend unfortunately. I am motivated to get this done though and will give an update by mid next week

bitwiseman · 2015-11-16T16:51:32Z

No problem. I (obviously) know how it goes. Thanks!

olsonpm · 2015-12-14T08:11:28Z

@bitwiseman - Took a good stab at this tonight, ran into a case I'm unsure about:

When operator_position is set to remove_newline What should the following resolve to?

var a = b

=== c;

The answer depends on whether you want preserve_newlines or operator_position to take precedence. This if statement would need to check for opt.operator_position !== OPERATOR_POSITION.remove_newline if we want the newlines to collapse in the above case.

I'll bump this in two weeks if I don't hear back.

bitwiseman · 2015-12-14T22:02:09Z

This is why I didn't include remove_newline as an option in my list above. 😄

I figured preserve_newlines would take precedence: if set to false existing newlines are ignored, if set to true the behavior can be customized.

Unless you feel we really need remove_newline, I'd drop it. It seems like something most people would not want - "keep newlines in general, but remove them when they occur before or after an operator"?

olsonpm · 2015-12-24T17:35:27Z

Ah - that's what I get for skimming. Thanks much for the clarification.

olsonpm · 2015-12-25T17:41:55Z

@bitwiseman - I feel pretty good about this second draft. Ensuring the operator position logic only applies when preserve_newlines is true simplifies everything, especially tests.

Honestly I was very surprised our breaking change didn't cause a bunch of tests to break. I was expecting to have to go through and fix a ton of them.

By the way, instead of looking at the diff between the first -> second draft, it may make more sense to diff master -> second draft. My branch should be rebased to the current master.

olsonpm · 2016-02-03T18:43:22Z

*Rebasing and cleaning up the commit message now.

If there's anything about my commit message you want changed, it would be easiest if you just create another pull request and modify it yourself to save back-and-forth. I'm not concerned about having my name on anything.

bitwiseman · 2016-02-03T18:44:42Z

I'm not concerned about the message, except put an empty line below the title.

Title

Descrption
Descrption
Descrption

olsonpm · 2016-02-03T19:01:26Z

Alright - I looked at the readme and couldn't figure out why some cli options were left out so I'll leave it be for now.

- Add sanitization of operator_position option. Throws error if invalid. - Possible operator_position options are: 1) 'before-newline' (default) 2) 'after-newline' 3) 'preserve-newline' - Move some code around in handle_operator in order to separate conflicting logic now that operator newlines are accounted for. - Modify allow_wrap_or_preserved_newline to handle operator preserved newlines as well. - Add -> build tests - Within the tests, add an 'inputlib.js' file holding common input arrays. This was necessary due to the matrices not working cleanly with default behavior. Changes not directly relevant to operator_position - Add Object.values polyfill - Ignore intellijidea project file - Fix a few boolean variable assignments within if statements. Though not always the case, the expression within the if can be assigned directly to the variable

bitwiseman · 2016-02-03T19:22:06Z

The readme being out of date is totally possible. But unless it applies to this change getting it up to date is not needed.

bitwiseman · 2016-02-03T19:23:52Z

Ah I see I didn't update the help message in python. Meh, I'll get that in a minute.

olsonpm · 2016-02-03T19:25:54Z

Gotcha - and yeah I just thought updating the readme might make more sense in a separate commit to master.

first draft of operator_position functionality

bitwiseman · 2016-02-03T19:52:32Z

@olsonpm - thanks again for all your hard work on this. 👍

nerdpad · 2016-03-19T04:33:30Z

@bitwiseman what is the jsbeautifyrc option for this feature?

I added "operator_position": "preserve-newline" to my rc, still it inlines ternary operators.

olsonpm · 2016-03-19T17:37:49Z

@nerdpad - what version are you using?

nerdpad · 2016-03-21T14:37:19Z

@olsonpm I am using v1.6.2. Here is the output that I am getting:

Source:

const result = true
    ? true
    : false;

After format:

const result = true ? true : false;

.jsbeautifyrc

{
    "indent_size": 4,
    "indent_char": " ",
    "eol": "\n",
    "indent_level": 0,
    "indent_with_tabs": false,
    "preserve_newlines": true,
    "max_preserve_newlines": 2,
    "jslint_happy": false,
    "space_after_anon_function": false,
    "brace_style": "collapse-preserve-inline",
    "keep_array_indentation": false,
    "keep_function_indentation": false,
    "space_before_conditional": true,
    "break_chained_methods": false,
    "eval_code": false,
    "unescape_strings": false,
    "wrap_line_length": 0,
    "wrap_attributes": "auto",
    "wrap_attributes_indent_size": 4,
    "end_with_newline": true,
    "operator_position": "preserve-newline"
}

olsonpm · 2016-03-21T15:38:02Z

Hey thanks much - I'll take a look at this tonight when I'm back from work.

olsonpm · 2016-03-21T16:05:59Z

@nerdpad - 1.6.2 doesn't have this functionality. Please let me know what led you to believe this was implemented so we can update misleading documentation.

1.6.2 was committed jan 31, this patch was merged on feb 3. Not sure this patch will go in 1.6 as it does modify existing behavior - albeit that behavior is more a side effect than purposeful.

nerdpad · 2016-03-21T16:27:07Z

@olsonpm I saw the commit was merged, so I assumed it was available in the latest version. What I didn't check was the release date of 1.6.2. Sorry about that.

Do we know when this patch will get in the main release?

olsonpm · 2016-03-21T16:36:58Z

Not a problem - I only asked in case we did anything wrong.

and @bitwiseman will be able to answer that question.

bitwiseman · 2016-03-24T18:58:34Z

Doh, yeah, it went in just after 1.6.2. I need to get 1.6.3 published. 😄

nerdpad · 2016-05-16T20:18:22Z

@bitwiseman Please publish 1.6.3. :)

bitwiseman · 2016-05-19T20:58:52Z

Thanks for the reminder. 😄

bitwiseman · 2016-06-03T07:16:34Z

Done! Enjoy!

olsonpm mentioned this pull request Jul 16, 2015

Allow bool op first #611

Closed

bitwiseman reviewed Jul 20, 2015
View reviewed changes

olsonpm force-pushed the add-operator-position branch from f600f26 to fb4c5f9 Compare December 14, 2015 00:30

olsonpm force-pushed the add-operator-position branch from 0ee8f3e to 007c57d Compare February 3, 2016 19:00

olsonpm force-pushed the add-operator-position branch from 007c57d to d83308d Compare February 3, 2016 19:05

bitwiseman added a commit that referenced this pull request Feb 3, 2016

Merge pull request #735 from olsonpm/add-operator-position

5395e0d

first draft of operator_position functionality

bitwiseman merged commit 5395e0d into beautifier:master Feb 3, 2016

This was referenced Feb 3, 2016

Support newline before logical or ternary operator #605

Closed

ternary operations #670

Closed

olsonpm deleted the add-operator-position branch February 3, 2016 19:33

bitwiseman mentioned this pull request Mar 21, 2016

unary operators in multiline statement - before vs after #901

Closed

nerdpad mentioned this pull request Jun 6, 2016

Please update js-beautify to 1.6.3 Glavin001/atom-beautify#1005

Closed

laggingreflex mentioned this pull request Oct 9, 2016

Please update js-beautify to 1.6.3 victorporof/Sublime-HTMLPrettify#343

Closed

olsonpm mentioned this pull request Dec 29, 2016

Explaination of 'operator_position' is absent from README.md #1047

Closed

first draft of operator_position functionality #735

first draft of operator_position functionality #735

Conversation

olsonpm commented Jul 16, 2015

olsonpm commented Jul 16, 2015

bitwiseman commented Jul 20, 2015

bitwiseman Jul 20, 2015

Choose a reason for hiding this comment

olsonpm Jul 23, 2015

Choose a reason for hiding this comment

olsonpm commented Jul 20, 2015

bitwiseman Jul 20, 2015

Choose a reason for hiding this comment

olsonpm Jul 23, 2015

Choose a reason for hiding this comment

bitwiseman commented Jul 20, 2015

olsonpm commented Jul 21, 2015

olsonpm commented Jul 24, 2015

bitwiseman commented Nov 10, 2015

olsonpm commented Nov 10, 2015

bitwiseman commented Nov 10, 2015

olsonpm commented Nov 10, 2015

bitwiseman commented Nov 10, 2015

olsonpm commented Nov 10, 2015

bitwiseman commented Nov 12, 2015

olsonpm commented Nov 13, 2015

olsonpm commented Nov 16, 2015

bitwiseman commented Nov 16, 2015

olsonpm commented Dec 14, 2015

bitwiseman commented Dec 14, 2015

olsonpm commented Dec 24, 2015

olsonpm commented Dec 25, 2015

olsonpm commented Feb 3, 2016

bitwiseman commented Feb 3, 2016

olsonpm commented Feb 3, 2016

bitwiseman commented Feb 3, 2016

bitwiseman commented Feb 3, 2016

olsonpm commented Feb 3, 2016

bitwiseman commented Feb 3, 2016

nerdpad commented Mar 19, 2016

olsonpm commented Mar 19, 2016

nerdpad commented Mar 21, 2016

olsonpm commented Mar 21, 2016

olsonpm commented Mar 21, 2016

nerdpad commented Mar 21, 2016

olsonpm commented Mar 21, 2016

bitwiseman commented Mar 24, 2016

nerdpad commented May 16, 2016

bitwiseman commented May 19, 2016

bitwiseman commented Jun 3, 2016