Skip to content

Issue #291 better one line scope indentation #293

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

Merged
merged 1 commit into from
Feb 22, 2016

Conversation

camsteffen
Copy link
Contributor

No description provided.

@camsteffen camsteffen changed the title Issue #294 better one line scope Issue #291 better one line scope Sep 7, 2015
@camsteffen camsteffen changed the title Issue #291 better one line scope Issue #291 better one line scope indentation Nov 27, 2015
@qstrahl
Copy link
Collaborator

qstrahl commented Feb 22, 2016

@camsteffen Can you elaborate on what this change addresses? Also you'll need to rebase if you want it merged in

@davidchambers
Copy link
Collaborator

Presumably this solves the issue raised in #291:

if(1) return
    console.log('foo')

@qstrahl
Copy link
Collaborator

qstrahl commented Feb 22, 2016

@camsteffen It looks like you merged our develop branch into yours; you will have to undo that change with a git reset --hard b246a506bd1bdfa0827a9861707ffb0b6934b3c5 and instead do a rebase with git rebase refs/remotes/pangloss/develop You will have to resolve the resultant merge conflict.

@camsteffen
Copy link
Contributor Author

Thanks for helping with the rebase. Hopefully it is set to go now.

To recap the change, the regex now checks to see if there is a statement after the parenthesis for a if|for|while so that if(foo) return does not begin a one line scope. If a line ends with else, then it does begin a one line scope. This pattern could potentially break for something like while("(foo)" === bar) but that is an edge case.

@qstrahl
Copy link
Collaborator

qstrahl commented Feb 22, 2016

I'm willing to try it out for now. We can revert if breakages come up.

qstrahl added a commit that referenced this pull request Feb 22, 2016
Issue #291 better one line scope indentation
@qstrahl qstrahl merged commit 47b89fb into pangloss:develop Feb 22, 2016
@bounceme
Copy link
Collaborator

hey, some things are still being indented

function n (err) {
  if (err) return log(err)
    var act = 0
}

another example

function (opts) {
  function log (message) {
    console.error(message)
    if (opts) say(opts, message)
  }
client.on('error', function (message) {
  console.error('Error:', message)
})
}

@camsteffen
Copy link
Contributor Author

Unfortunately I think we have the best solution possible using regular expressions. It becomes an ultimatum between (1) falsely indent after if(foo) bar() or (2) falsely not indent after if(foo()). I prefer option 1, especially since it can be fixed with a semicolon: if(foo) bar();

@@ -66,7 +66,7 @@ let s:continuation_regex = '\%([\\*+/.:]\|\%(<%\)\@<![=-]\|\W[|&?]\|||\|&&\|[^=]
" TODO: this needs to deal with if ...: and so on
let s:msl_regex = s:continuation_regex.'|'.s:expr_case

let s:one_line_scope_regex = '\%(\<\%(if\|else\|for\|while\)\>\|=>\)[^{;]*' . s:line_term
let s:one_line_scope_regex = '\%(\<else\>\|\<\%(if\|for\|while\)\>\s*(.*)\)' . s:line_term
Copy link
Collaborator

Choose a reason for hiding this comment

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

unexpectedly this seems to be working:

let s:one_line_scope_regex = '^[})\]]*\s*\(if\|else\|while\|try\|catch\|finally\|for\|else\s\+if\)\s*_\=$'

gotten from https://github.com/jiangmiao/simple-javascript-indenter/blob/master/indent/javascript.vim#L350

@bounceme
Copy link
Collaborator

there isn't indentation when the curly braces are omitted and the statement goes to the next line

@camsteffen
Copy link
Contributor Author

can you give an example?

@bounceme
Copy link
Collaborator

  if (true)
  console.log("value");

that other indent script correctly indents here

@camsteffen
Copy link
Contributor Author

Your example indents correctly for me...

@bounceme
Copy link
Collaborator

did you replace line 69 with

let s:one_line_scope_regex = '^[})\]]*\s*\(if\|else\|while\|try\|catch\|finally\|for\|else\s\+if\)\s*_\=$'

@camsteffen
Copy link
Contributor Author

No, but now I'm confused. Why are we looking at this other line of code if it doesn't work?

@bounceme
Copy link
Collaborator

the indent script that it is copied from, jiangiao's, does correctly indent these different variations of ifs.maybe we can get a similar behavior by looking at other parts of that script to see what he did

@camsteffen
Copy link
Contributor Author

Sorry, it doesn't solve the problem that I described before:

It becomes an ultimatum between (1) falsely indent after if(foo) bar() or (2) falsely not indent after if(foo()). I prefer option 1, especially since it can be fixed with a semicolon: if(foo) bar();

Here's another way to look at it. Using regular expressions, there is no way to detect whether parenthesis are matching. It can't see if if ( matches with ) at the end of the line.

@bounceme
Copy link
Collaborator

well, it may be possible if we try with :h searchpair()

@camsteffen
Copy link
Contributor Author

Good find, I didn't know about that. Maybe I'll look into it when I have some time. It would be more than a one-liner fix.

@bounceme
Copy link
Collaborator

This is what I think would be easiest,though not elegant:

let s:one_line_scope_regex = '\%(\%(\<else\>\|\<\%(if\|for\|while\)\>\s*(\%([^()]*\|[^()]*([^()]*)[^()]*\))\)\|=>\)' . s:line_term

This can be extended to allow nesting more than two pairs

@bounceme
Copy link
Collaborator

last comment showed two pairs nested,here's what 4 looks like...

let s:one_line_scope_regex = '\%(\%(\<else\>\|\<\%(if\|for\|while\)\>\s*(\%([^()]*\|[^()]*(\%([^()]*\|[^()]*(\%([^()]*\|[^()]*([^()]*)[^()]*\))[^()]*\))[^()]*\))\)\|=>\)' . s:line_term

bounceme added a commit to bounceme/vim-javascript that referenced this pull request Apr 12, 2016
pangloss#293
this will detect the start and end parentheses,allowing statements with parentheses to come after the condition parens.This regex can detect with up to 4 levels of nesting.
@bounceme bounceme mentioned this pull request Apr 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants