search

emacs-ess / ESS

Emacs Speaks Statistics: ESS
https://ess.r-project.org/
GNU General Public License v3.0
613 stars 160 forks source link

Extended support for function shorthand notation #1282

Closed maxecharel closed 2 months ago

maxecharel commented 4 months ago

Adds font-locking support for R function shorthand notation \() and for names of functions defined using this shorthand notation. Also makes the shorthand notation a recognized function pattern.

maxecharel commented 3 months ago

Hi @lionel- , sorry for the delay before answer. I will add to the existing unit tests for font-locking end of this week. I will get back to you. Thanks for the great work with ESS !

maxecharel commented 3 months ago

Hi @lionel- , your feedback would be welcome concerning the two following issues I encountered when adding to the unit tests:

  1. while \() passes the ess-test-r-fontification-keywords-fn-test test, I had to go for a 'trial and error' approach that I describe below. To investigate the reasons why it failed in the first place, I would like to get access to the ERT test buffer (not just the results) to see how the string is printed. Any idea how to do so?
  2. when function name is backquoted, it is not highlighted (I realized that when trying to add to ess-test-r-fontification-keywords-disabled-backquoted-defun), i.e.: backquoted_name_for_shorthand_notation_no_fl I failed to identify the problem. I will carry on investigating in the following days, but maybe you have some hints on the nature of the issue?

Concerning (1), at first ess-test-r-fontification-keywords-fn-test failed. It was rather surprising given that manually executing (face-at-point) in my minimal Emacs testing config worked as expected: etest_manual_testing_ok Anyway, ERT indicated a failure whenever the relevant string was set (in the :case string) to \\\\(), \\() or even \() (with the pilcrow character located at the relevant place depending on :case or :result).

My first guess was that the problem comes from the way ERT, based on the :case of ess-test-r-fontification-keywords-fn-test, prints the string representing \() in a test buffer. After some trials and errors, I ended up using group delimiters --- i.e. I went for \\(\\\\\\)() --- and it passed the test. Yet I am not satisfied with this, and I would like to get a deeper understanding of what happened exactly. Is there any way I can get access to the actual test buffer in which the expressions contained in :case are printed and actually tested? Or am I missing the point and there is no creation of such buffer?

Thanks. Best, Max

lionel- commented 3 months ago

@maxecharel I will try to find time to take a look this week.

maxecharel commented 3 months ago

@lionel- some updates (to keep you posted and as notes for myself):

  1. I understood why backquoted 'names' of functions defined with \() were not fontified; solved with commit 5c24a72 by modifying ess-r-font-lock-syntactic-face-function after_modifying_ess-r-font-lock-syntactic-face-function
  2. I discovered an issue with my initial modification of ess-R-function-name-regexp while I was investigating the previous issue and fixed it with commit 49bcd58: before_additional_adjustment_ess-R-function-name-regexp after_additional_adjustment_ess-R-function-name-regexp

One question remains, and one has been added (this one will certainly require your feedback at some point, sorry for that)

  1. how to get access to the ERT test buffer (nothing new here; I'll try to answer that later this week while reruning the tests, but if and only if you find time, your feedback is more than welcome)
  2. definitions of ess-r-mode and ess-r-transcript-mode (as well as inferior-ess-r-mode) all reference ess-r-font-lock-syntactic-face-function, but they (except the latter mode) also set a local add-log-current-defun-header-regexp including a string in the spirit of "<- function". However, here it looks like we enter a world that goes beyond font-locking (and therefore the scope of this PR). When you have the opportunity, could you just confirm this? Anyway I will set that aside for now, and will try to find the time to dig into it later. Cheers!
lionel- commented 2 months ago

Sorry I don't have any bandwidth for this :/

Let's just merge your work.