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

Respect indentation for visually selected ":Prettier" #78

Closed
wants to merge 1 commit into from

Conversation

davidsu
Copy link

@davidsu davidsu commented Nov 8, 2017

fixes #75

@mitermayer
Copy link
Member

Hi @sudavid4 ,

That's awesome! Thank you so much for submitting this PR! Will test it as soon as I land some fixes for prettier 1.8 compatibility.

@mitermayer mitermayer self-requested a review November 8, 2017 19:24
@mitermayer
Copy link
Member

Thanks for submitting this PR, it seems that your fix was only applied to synchronous formatting we should however put in a place where both sync an async could benefit from it.

Copy link
Member

@mitermayer mitermayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this moment the fix is only applied to synchronous formatting, we should also support asynchronous formatting.

@davidsu
Copy link
Author

davidsu commented Nov 9, 2017

fixed this, moved the code into s:Apply_Prettier_Format
also using a:startSelection instead of visualSelectionStart ('<) to get startLine

@mitermayer
Copy link
Member

Awesome!!

Will test this out tomorrow then if all is good will merge it! thank you so much

@mitermayer
Copy link
Member

Hi @sudavid4,

Tried testing this, but i think this behaviour may not be always what we want. An example:

this output with visual selection:

  function a () {
      const a = 123; }

would create this:

  function a() {
    const a = 123;
}

I think introducing something like this would make vim-prettier differ too much from prettier itself and could cause some hard to debug bugs like the above.

@davidsu
Copy link
Author

davidsu commented Nov 11, 2017

Please see if this is better, taking into account added (or subtracted for that matter) lines in buffer to calculate line end

@mitermayer
Copy link
Member

hi @sudavid4,

Been thinking about it, I think this sort of API would make sense to even be included in prettier itself. Was thinking that if the CLI supported an API like line start and line end setting we could have something like this:

formatting this fixture with this hypothetical command prettier --line-start=5 --line-end=5 foo.js:

1
2  const b
3    = 2;
4  function a() { 
5      const d = (blah) => { return 3 + 5 }; return d;
6  }

could then output:

1
2  const b
3    = 2;
4  function a() { 
5    const d = blah => {
6      return 3 + 5;
7    };
8    return d;
9  }

formatting just the selected area.

I think this feature makes more sense to be implemented in prettier itself since they can better understand surrounding code and better chose spacing/formatting with all contexts involved.


I am happy to contribute this feature for prettier itself since I imagine this could be a useful feature for other editor integrations.

Any thoughts around it ?

cc @lydell @azz @ikatyang @duailibe

@mitermayer
Copy link
Member

Saying that, will test this PR today as it is since that is not a blocker and we could get this merged. We can always revisit this workaround implementation later in favor of the prettier CLI args if they can implemented in there.

@ikatyang
Copy link
Member

Prettier already supports --range-start and --range-end, aren't they the same feature?

@ikatyang
Copy link
Member

demo.js

const b
  = 2;
function a() { 
    const d = (blah) => { return 3 + 5 }; return d;
}

command

prettier demo.js --range-start 32 --range-end 84

stdout

const b
  = 2;
function a() {
    const d = blah => {
      return 3 + 5;
    };
    return d;
}

@mitermayer
Copy link
Member

@ikatyang calculating the range on viml is not ideal, would prefer a higher level API where we can supply just the line itself

@mitermayer
Copy link
Member

Sorry still haven't found time to test this PR will try to do that tonight! Apologies for the delay!

@mitermayer
Copy link
Member

Hi @sudavid4 ,

Just tested this and still fail on some cases:

output

@mitermayer
Copy link
Member

We could try to use the API mentioned by @ikatyang using vim line2byte(line('.')) + col('.') - 1 to get the byte position.

@mitermayer
Copy link
Member

Will experiment with this as soon as i get some free time

@mitermayer
Copy link
Member

I have been pretty busy lately but will get back in this PR next week and make sure it will get merged in some form!

@mitermayer mitermayer mentioned this pull request Mar 25, 2018
13 tasks
@mitermayer
Copy link
Member

mitermayer commented May 5, 2018

Hi @docwhat

We could try to do this for 1.0 using line2byte to get the starting and ending bytes for N and Y and add --range-start N --range-end Y cli args for prettier, what are your thoughts around it ?

I have been toying around with a helper for getting bytes range:

" Returns [start, end] byte range
function! s:getByteRange() abort
  let l:range = []
  let l:start = getpos("'<")
  let l:end = getpos("'>")

  call add(l:range, line2byte(l:start[1]) + l:start[2] - 1)
  call add(l:range, line2byte(l:end[1]) + l:end[2] - 1)

  return l:range
endfunction

The above works well for normal visual selection and visual block selection but not for visual line selection. I may be missing something but for visual line selection it returns MAX_INT (2147483730)

so running the visual selection on the above snippet would generate [32, 2147483730] instead of [32, 84] like for the other cases

Also if that is something you would want to tackle or have interest feel free to let me know

@mitermayer
Copy link
Member

on :help getpos within vim there is a note:

Note that for '< and '> Visual mode matters: when it is "V"
(visual line mode) the column of '< is zero and the column of
'> is a large number.

@mitermayer
Copy link
Member

This is an example of implementation that made that work https://github.com/farazdagi/vim-go-ide/blob/master/bundle/pristine/vim-go/autoload/go/oracle.vim#L65

@docwhat
Copy link
Contributor

docwhat commented May 5, 2018

I'm not sure that you should be converting to bytes, since the documentation for prettier says it takes characters. Bytes would mess up when using multi-byte utf-8 characters (for example).

For the visual selection, can't you just use visualmode() or mode() to detect V (visual-line-selection) and then move the end position to row + 1 and column = 0?

@mitermayer
Copy link
Member

Thanks for that info @docwhat, Do you know a better way on vim to gather character instead ? Do you think you would be interested / have some time to tackle this issue for our 1.x branch ?

@mitermayer mitermayer added this to Doing in vim-prettier 1.0 May 16, 2018
@mitermayer mitermayer moved this from Doing to Backlog in vim-prettier 1.0 May 16, 2018
@mitermayer
Copy link
Member

Fix this on branch 1.x (ec6ede9)


Important notes:
Using the prettier api for --range-start and --range-end had some downsides:

  • sending larger buffer to prettier
  • requires the whole file to be valid even when formatting just portion of the file.

Details:
I kept the old behaviour as the default, but made it configurable via a flag, so users can make partial formatting enabled for them if they wish by simply:

let g:prettier#partial_format=1

I also created 2 helper commands :PrettierPartial and :PrettierFragment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Formatting with visual selection doesn't respect indentation
5 participants