D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  Will do. Thanks!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann added a comment.


  I normally follow the steps on
  
  https://community.kde.org/Guidelines_and_HOWTOs/Build_from_source
  
  and just do kdesrc-build ktexteditor after the initial setup.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  I... git cloned ktexteditor and ran `cmake .`? Is there a better way? Sorry, 
I'm a real newb in this as you can probably tell! I'm not a dev by training.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann added a comment.


  And btw., thanks that you work on that stuff!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann added a comment.


  Hi,
  the message means you need to update the extra-cmake-modules.
  How did you compile the stuff? If you use kdesrc-build, I would just let that 
run once more.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  OK. So, that'll require compile ktexteditor from source. I should be able to 
manage that (I guess?). I'll report here if it gives me too many headaches. I 
tried today, but gave up because of an issue with ECM (?) version. I got:
  
Could not find a configuration file for package "ECM" that is compatible
with requested version "5.50.0".

The following configuration files were considered but not accepted:

  /usr/share/ECM/cmake/ECMConfig.cmake, version: 5.49.0
  
  I'll find whether I can circumvent/fix this issue somehow.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann added a comment.


  You might need a .kateconfig in the R folder like in e.g. the cstyle folder 
which tells "use R highlighting and R indenter"

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann added a comment.


  I added these lines
  
  https://commits.kde.org/ktexteditor/784302a921b8e1fafd6d32cf172552eaea1775bf
  
  If you do make, you can later in the build folder run:
  
  ./bin/kateindenttest
  
  ATM it will tell at the end:
  
  SKIP   : IndentTest::testR() 
/home/cullmann/local/kde/src/frameworks/ktexteditor/autotests/input/indent/R/ 
does not exist
  
Loc: 
[/home/cullmann/local/kde/src/frameworks/ktexteditor/autotests/src/script_test_base.cpp(89)]

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann added a comment.


  Ok,
  
  here the basic idea:
  
  in autotests/src/identtest.cpp and .h
  
  > in .h
  ===
  
  add some
  
void testR_data();
void testR();
  
  > in .cpp
  =
  
  add some
  
  void IndentTest::testR_data()
  {
  
getTestData("R");
  
  }
  
  void IndentTest::testR()
  {
  
runTest(ExpectedFailures());
  
  }
  
  Then, after make, you should be able to run
  
  ./bin/kateindenttest
  
  and it should start to use the test data in the R dir

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  That's the thing: I don't know how to run the test. If it requires dealing 
with cmake and stuff, that's take me a while to figure out. Is there any simple 
documentation about it?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann added a comment.


  You don't need to commit them.
  You just can add them locally test by test and after each one run the indent 
test locally.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  So, basically, I just write a bunch of tests, commit them here and we'll 
debug them if needed from here?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann added a comment.


  The easiest way is just to put the test in the autotests input like for other 
highlighters and then run the indent test, that will pick this up and tell you 
what happens.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  Say that I have the files for a unit test (origin, input.js and expected). 
How can I check the code for the unit test? Is there a way to simply execute 
the javascript code and verify it does what I expect? It would be a shame to 
have the unit test fail because of the code test itself.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  I might get to that around September (no promise). If people are willing to 
help, my not-unit-at-all test code for indentation can be found here:
  https://github.com/devillemereuil/rindent/blob/master/example.R
  
  Automatic indent should not impact the code (this is how I test my commits), 
meaning it is already indented as the script is willing to indent.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Dominik Haumann]

dhaumann added a comment.


  @devillemereuil That is good to hear. If you can, feel free to paste a 
MIT-licensed snippet of R-code with a function, a loop, a if-condition etc. 
Maybe we find the time to turn this into a unit test. The code does not need to 
run, it just needs to be syntactically correct.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  Just to say: this didn't fall off my radar. I'm just incredibly busy nowadays 
and it never quite make it to the top of my todo list... But I think about 
doing it regularly, which is a good sign it'll happen!

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:467cf37fd04f: Indentation script for R (authored by 
cullmann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12587?vs=33259=39754

REVISION DETAIL
  https://phabricator.kde.org/D12587

AFFECTED FILES
  src/script/data/indentation/r.js

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward, cullmann
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars

D12587: Indentation script for R

[Christoph Cullmann]

cullmann added a comment.
Herald edited subscribers, added: kde-frameworks-devel, kwrite-devel; removed: 
Frameworks.


  Given it seems nobody has time to do tests, I will merge this.
  We can still remove it again but just letting rot this nice contribution in 
the phabricator won't help any R user.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward
Cc: kwrite-devel, kde-frameworks-devel, cullmann, tfry, dhaumann, michaelh, 
kevinapavew, ngraham, bruns, demsking, sars, #frameworks

D12587: Indentation script for R

[Thomas Friedrichsmeier]

tfry added a comment.


  @devillemereuil This was meant to replace calcMismatchIndent(), or at least 
the upper part of it. But don't let me distract you, here, this is 
non-essential. Just focus on the unit tests for now.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward
Cc: tfry, dhaumann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  I will have a go at it. It doesn't seem very complicated indeed, but I just 
don't have much time right now for this.
  
  @tfry I'm not sure what your pseudo-code is intended to replace? The parts 
using countClosing? FYI, these snippets of code were copied from the Python 
script. I learned JS on the go here, which is why I'm using very little 
idiosyncrasies from the language (apart from what I saw on the other scripts, 
mainly Python and Cstyle, with a few things picked up on Stack Overflow, I've 
got to admit...), so it's quite possible the script is not as elegant as it 
could be.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward
Cc: tfry, dhaumann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars

D12587: Indentation script for R

[Dominik Haumann]

dhaumann added a comment.


  @devillemereuil If I remember correctly, you do not have to run all tests. It 
is enough to start the indenter test (look into the bin folder) and add as 
parameter you indent test folder (relative, e.g. cstyle). And, what Thomas says 
is correct. Typically a test case only contains very few lines to test a 
specific case. I think once you get the first one running, it will be easy for 
you to add more.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward
Cc: tfry, dhaumann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars

D12587: Indentation script for R

[Thomas Friedrichsmeier]

tfry added a comment.


  Regarding the unit tests: Take a look at e.g. 
https://cgit.kde.org/ktexteditor.git/tree/autotests/input/indent/cmake/enter1 . 
You start with an initial text "origin", then emulate some input "input.js", an 
specify the expected state at the end "expected".
  
  Again, I have no experience with indentation scripts, so no good idea what 
they should look like. A nitpick / idea is inlined.

INLINE COMMENTS

> r.js:174
> +// Returns -1 if nothing was found
> +function findAlignOperator(lineNr, pos) {
> +var lineString = document.line(lineNr);

I wonder if this cannot be simplified (but not sure I understand the complete 
logic). Pseudo-code:

  var brackets = new Array();
  for ([iterating backwards, skipping strings and comments]) {
 var current_char = [...];
 var closing_type = closings.indexOf(current_char);
 if (closing_type >= 0) {
   brackets.push(current_char);
 } else {
   var opening_type = openings.indexOf(current_char);
   if (opening_type >= 0 && brackets.pop() != current_char) {
  [mismatch: return]
   }
   [...]
 }
  }

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward
Cc: tfry, dhaumann, #frameworks, michaelh, kevinapavew, ngraham, bruns, 
demsking, cullmann, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil added a comment.


  In D12587#255484 , @dhaumann wrote:
  
  > I think this is very cool. But there is one thing we require for new 
indenters: unit tests.
  >
  > There is a subfolder autotest in the KTextEditor git module. Could you have 
a look into this? Without such automated tests, we will likely introduce 
regressions in future updates of the indenter. If you need help, please let us 
know :-)
  
  
  Hi,
  
  OK, I think I see how it works by looking at the folder. Though I'm not a 
professional dev, so the notion of unit test is a bit blur for me (I understand 
it's about testing a very precise case with a template output to compare to?). 
I can have a go at it, but it might take some time...
  
  No problem for changing the licence and the version requirement. I just 
copied what I found on the Python script, as far as I remember...

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward
Cc: dhaumann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
cullmann, sars

D12587: Indentation script for R

[Dominik Haumann]

dhaumann added a comment.


  I think this is very cool. But there is one thing we require for new 
indenters: unit tests.
  
  There is a subfolder autotest in the KTextEditor git module. Could you have a 
look into this? Without such automated tests, we will likely introduce 
regressions in future updates of the indenter. If you need help, please let us 
know :-)

INLINE COMMENTS

> r.js:4
> +"author": "Pierre de Villemerereuil ",
> +"license": "LGPL",
> +"revision": 1,

Would you also agree to use MIT license? MIT ist preferred for Javascript and 
XML highlighting files.

> r.js:6
> +"revision": 1,
> +"kate-version": "5.1",
> +"indent-languages": ["R", "R Script", "Script R"]

I believe 5.0 is good enough. There were no breaking changes between 5.0 and 
5.1.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

To: devillemereuil, #ktexteditor, #rkward
Cc: dhaumann, #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, 
cullmann, sars

D12587: Indentation script for R

[Pierre de Villemereuil]

devillemereuil created this revision.
devillemereuil added reviewers: KTextEditor, RKWard.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.
devillemereuil requested review of this revision.

REVISION SUMMARY
  This is a javascript indentation script for the statistical language R. This 
script is for all software using KTextEditor (e.g. Kate), but more specifically 
for RKWard, which is an IDE for R.
  
  I tested it for corner cases for some time so it should be quite robust, 
although my ability to find corner cases alone is obviously limited by a lack 
of diversity in coding styles.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D12587

AFFECTED FILES
  src/script/data/indentation/r.js

To: devillemereuil, #ktexteditor, #rkward
Cc: #frameworks, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, 
sars, dhaumann