search

nim-lang / nim-mode

An emacs major mode for the Nim programming language
139 stars 46 forks source link

indentation with smie.el (but need emacs 25.1) #64

Closed yuutayamada closed 8 years ago

yuutayamada commented 8 years ago

Hi, I implemented indentation logic using smie.el.

I think it's still missing enough tests and currently I used emacs' new package (subr-x). So, almost users can't use this immediately, but I thought it's worth to let you know:)

Some tricky indentations are available because of smie.el:

var foo = if condition:
            echo "indent starts here"
          else:
             ...

How to use:

(setq nim-use-smie-indent t) ; <- set this before loading nim-mode

Thanks

yuutayamada commented 8 years ago

FYI, above failing tests are OK only in my local machine... Provably I need to add emacs 25's virtual environment. (but still there is no bin data in EVM.)

reactormonk commented 8 years ago

You sure it's not 24.4? According to http://emacsredux.com/blog/2014/02/02/a-peek-at-emacs-24-dot-4-new-string-manipulation-functions/ If 24.4 is the minimum version, add it to the readme and change the travis build. Or make some code that disables this feature if you're using an older version of emacs and prompt the user to upgrade.

Also, the travis tests are essential. As expected, it can't load subr-x on 24.3, but 24.4 gives error: (void-function if-let).

yuutayamada commented 8 years ago

the if-let function was introduced at emacs 25.1 according to emacs NEWS in my emacs. I might change that dependencies if I have time later, but currently nim-smie's parsing rule is complicated, so I would use if-let until then...

reactormonk commented 8 years ago

Mind using the if-let from https://github.com/magnars/dash.el#-if-let-var-val-then-rest-else ?

yuutayamada commented 8 years ago

Oh nice! I'll take a look at it!

yuutayamada commented 8 years ago

Hi, somehow I was changing helper function for old indentation and due to that test was failing... (sorry I didn't notice until I turn on compiling .el file for test)

And about using dash's -if-let, I tried it, but I couldn't pass current tests for smie. So instead I ported same code from subr-x on emacs 25.1. I seem it's passing on emacs 24.4 (but 24.3 was failed on travis)

FYI, I referred this approach: https://github.com/clojure-emacs/cider/issues/1348

yuutayamada commented 8 years ago

just added emacs 24.5's configuration for travis

reactormonk commented 8 years ago

Why the conditional before using it? What would be the downside to replacing the current indent system with smie.el and dumping the old code?

yuutayamada commented 8 years ago

just because I don't test nim's indention enough and currently the test is failing on emacs 24.3. But I don't have any objection supporting this package from emacs 24.4 and remove old indentation code.

reactormonk commented 8 years ago

Sure, feel free to go to 24.4 if it means we can reduce code size. It's approx. a year old. https://www.gnu.org/software/emacs/history.html

Use the old test cases for SMIE. Or even better, run all the indent tests on both the old and new code.

yuutayamada commented 8 years ago

Actually I almost copied from old indentation tests, so I think I can omit old indentation tests, which can reduce code size more. But, I seems using SMIE has little bit problem when I test it from compiled .el files even though it's manually working fine. So, I comment out testing from compiled codes. Do you mind with this? (065d9ed)

reactormonk commented 8 years ago

What's the exact issue?

yuutayamada commented 8 years ago

Indentation tests fail randomly. https://travis-ci.org/nim-lang/nim-mode/jobs/98057449

I implemented to prevent current indentation if previous line is empty and there were some sort of tests, but sometimes tests fail or pass by just adding other tests...

yuutayamada commented 8 years ago

I made sure failed tests manually and it works fine (I'm using byte compiled nim-mode), so not sure what's the problem.

reactormonk commented 8 years ago

Are the fails consistent when adding a test? Maybe some internal state?

yuutayamada commented 8 years ago

It could be... sometimes it just solved moving a function top to bottom. FYI, it can avoid if I divide functions to separate files, but that case we can't check functions combined state.

reactormonk commented 8 years ago

Can't reset the state somehow?

yuutayamada commented 8 years ago

Hi, somehow I fixed tests (I just split toplevel procs). But, I realized two missing features compared to old indentation system.

  1. electric-colon
  2. cycle indent

I'm not sure how important those functions, so I still kept old indentation code.

Can you merge in this state? I would send PR separately to remove old indentation code when I'm ready. (People can go back if my smie indent isn't good enough)

yuutayamada commented 8 years ago

Sorry, seems I need to implement a function for show-paren because smie provides a function to get data for show-paren-mode and it doesn't work well currently. If I turn off, the indentation is working (I think)

yuutayamada commented 8 years ago

Oh, never mind, I could bind show-paren-mode's default function instead of smie's. The downside of this is just you can't see highlight for matching pairs like if and else without parenthesis.