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

Make ess-r-flymake optional? #490

Open
unixpipecentipede opened this issue Feb 14, 2018 · 34 comments
Open

Make ess-r-flymake optional? #490

unixpipecentipede opened this issue Feb 14, 2018 · 34 comments

Comments

@unixpipecentipede
Copy link
Contributor

Since introduced, ess-r-flymake is enabled by default in R-mode buffers (emacs26 only).

It would feel more emacsy imo if this feature could be deactivated, along the lines of ess-use-company, ess-use-eldoc, etc. AFAIK, currently it cannot be deactivated in init.el before loading ess-r-flymake.el, where the activation is added to ess-mode-hook.

@vspinu
Copy link
Member

vspinu commented Feb 14, 2018

Indeed. I was thinking along the same lines. In the meanwhile I keep deactivating linters to make the default minimally intrusive.

@vspinu vspinu closed this as completed in 62c7705 Feb 14, 2018
@mmaechler
Copy link
Member

On those platforms where I'm on Fedora 28 w/ emacs 26 (not my desktop for now, but soon), I still get bothered a lot by messages (and corresponding short "timeouts"!)

error in process sentinel: Invalid function: flymake-log [2 times]

until I turn off flymake ..
Could flymake check "itself" before it starts itself in such cases, or then turn itself off after the first warning?

@mmaechler mmaechler reopened this Sep 1, 2018
@lionel-
Copy link
Member

lionel- commented Sep 1, 2018

See also #545 for a tentative fix.

@mmaechler
Copy link
Member

?? #545 really - My problem is w/ emacs 26 (= for me the only env where flymake is available)

@lionel-
Copy link
Member

lionel- commented Sep 1, 2018

The error your mention happens when ESS is compiled with an old Emacs and run on a newer Emacs. The fix in 545 prevents ess-flymake to be compiled on old Emacs.

@lionel-
Copy link
Member

lionel- commented Sep 1, 2018

(That's because flymake-log is a macro, so if it's not available at compile-time Emacs assumes it's a function, hence the runtime error)

@vspinu
Copy link
Member

vspinu commented Sep 1, 2018

It might help to add a few declare-functions for the sake of emacs 25 byte-compiler. I would not change this in Makefile because that doesn't work on MELPA.

@lionel-
Copy link
Member

lionel- commented Sep 1, 2018

We are talking about runtime errors, not compile-time errors.

@lionel-
Copy link
Member

lionel- commented Sep 1, 2018

We could record the Emacs version at compile-time in a constant, then we'd disable flymake if ESS was compiled on a too old Emacs.

@mmaechler
Copy link
Member

Thank you Lionel... it's amazing how I'm again and again falling in the same trap
(when working in machines with different versions of default emacs, but the same home directory).

Your proposal (recording emacs-version at compile-time and use that in addition in the decision to enable/disable flymake) seems cool and could be re-used for similar cases in the future...

@vspinu
Copy link
Member

vspinu commented Sep 1, 2018

@lionel- I understand what you are talking about. Runtime errors occur because during compilation those functions are not available. If they were declared then even if ess-flymake is compiled with emacs 25 it should in principle work on 26. I am not 100% confident this will work but at least it's worth a try.

@vspinu
Copy link
Member

vspinu commented Sep 1, 2018

Nevermind. It won't work. flymake-log is a macro (missed that part). Would need to redefine the full macro locally.

@vspinu vspinu closed this as completed in 69b21fb Sep 1, 2018
@vspinu
Copy link
Member

vspinu commented Sep 1, 2018

Fixed along the lines that Lionel suggested with an extra warning so that people on emacs > 25 won't get surprised why r-flymake is not loaded.

@eddelbuettel
Copy link
Contributor

eddelbuettel commented Oct 5, 2019

Warming up old thread as my Emacs buffers now have too much "green" in them:

It would feel more emacsy imo if this feature could be deactivated

I would still like that. Now that lintr 2.0.0 is on CRAN, this seems to auto-activate which I find too strong. I learned that I can turn lint() off by adding ~/.lintr with NULL assignments to all default linters -- which ESS promptly ignores. Hobbling something together along the same lines in ~/.emacs did not work (see below). Nor did customization (which I have warmed up to in the last few years, a nice feature overall).

To sum up, I am on a machine with only emacs 26.1 but I would like to tell flymake to be more quiet. I don't mind opting into one or a few linters, but overall this is way too opionated and should not be on by default. Happy to make it a new issue if you prefer.

;; 2019-10-05 ess and flymake
(setq ess-r-flymake-linters '(
    "absolute_paths_linter=NULL"
    "assignment_linter=NULL"
    "closed_curly_linter=NULL"
    "commas_linter=NULL"
    "commented_code_linter=NULL"
    "infix_spaces_linter=NULL"
    "line_length_linter=NULL"
    "no_tab_linter=NULL"
    "object_usage_linter=NULL"
    "camel_case_linter=NULL"
    "snake_case_linter=NULL"
    "multiple_dots_linter=NULL"
    "object_name_linter=NULL"
    "object_length_linter=NULL"
    "open_curly_linter=NULL"
    "single_quotes_linter=NULL"
    "spaces_inside_linter=NULL"
    "spaces_left_parentheses_linter=NULL"
    "trailing_blank_lines_linter=NULL"
    "trailing_whitespace_linter=NULL"
))

Edit: And for completeness, behaviour unchanged between the (distro-packaged) 18.10.2 and a fresh elpa version 20190928.755.

@eddelbuettel
Copy link
Contributor

Found it, eventually, in the source. This seems to do it:

;; 2019-10-05 ess and flymake
(setq ess-use-flymake nil)

@mmaechler
Copy link
Member

Thank you, Dirk. For me as well, turning it on by default is "not nice" .. and I will turn it off by default myself. It slows down / rarely almost stalls emacs, for me too.
The argument for having it on by default has been that "only then" ESS users see and use it..

OTOH it seems that at least 3 very active R users (you, me, Marius) have been frustrated enough by its default presence and partly negative impact to mention it repeatedly ...

@lionel-
Copy link
Member

lionel- commented Oct 7, 2019

FWIW I disabled it as well. But we are probably not the natural target for a linter anyway since we're all conscientious about writing well punctuated code :-)

If it slows down / stalls Emacs, perhaps this feature should be implemented asynchronously? I've been having similar trouble with eldoc, moving the cursor inside a function call like rstanarm::stan_lm() with a freshly restarted R stalls Emacs because rstanarm takes a long time to load. Using a separate session for eldoc and completion might be tricky because it's not easy to figure out when the session should be restarted when a package has been reinstalled etc. However it doesn't sound like that constraint applies for linting, so it'd be easier to implement concurrently.

@eddelbuettel
Copy link
Contributor

(Should I open a new / fresh ticket ?)

The two big frustrations for me are

  • on by default seems wrong, and how to turn it off is poorly to not at all documented
  • configuration within emacs ignores the per machine configuration via e.g. ~/.lintr

and in a way the second part is as bad / worse. If I had a way to gently move towards using it that was consistent with command-line or R session use I might well be more inclined to use it. The fact that I first had to learn how to disable linting -- only to then learn that ESS ignores that configuration -- seems weird.

@jabranham
Copy link
Contributor

jabranham commented Oct 7, 2019 via email

@eddelbuettel
Copy link
Contributor

eddelbuettel commented Oct 7, 2019

Sure it's set. It's a UNIX machine:

edd@rob:~$ echo $HOME
/home/edd
edd@rob:~$ 

I had ~/.lintr placed, checked that it (correctly) suppressed all output when "linting" manually in R, yet this did not tame Emacs+ESS at all. Even reinstalled ESS from melpa just in case. No mas. Hence my unhappyness.

@nverno
Copy link
Contributor

nverno commented Oct 7, 2019

I agree it would be more emacsy to be off by default. I believe it's the only library I use out of a couple hundred that loads flymake by default (which I don't use either). In general, if an optional feature requires loading a package, I would prefer it be opt-in.

@mmaechler mmaechler reopened this Oct 7, 2019
@jabranham
Copy link
Contributor

@eddelbuettel - I just pushed a fix for finding .lintr in HOME. Can you check and see if that fixes your issue?

@eddelbuettel
Copy link
Contributor

I'd love to! Maybe by this eve. Do you think I can take a shortcut and 'hand-apply' this 18.10.2, or do you prefer I do an install from the repo?

@jabranham
Copy link
Contributor

I'm guessing the diff should apply cleanly but haven't checked.

@eddelbuettel
Copy link
Contributor

My first attempts with 18.10.2 was not successful. Tickled another message about project root or something when I loaded an R file from a repo. :-/

@jabranham
Copy link
Contributor

jabranham commented Oct 10, 2019 via email

@eddelbuettel
Copy link
Contributor

I did, and I can't as I reverted. I need a working setup.

I also wasted another thirty minutes trying another approach of packaging the current repo which failed over a mess with ess-julia.

So zero for two.

@eddelbuettel
Copy link
Contributor

eddelbuettel commented Oct 12, 2019

Gave it another spin, using the current git repo code (and wrapped in a .deb package).

Good news: It is more silent now.
Bad news: It is always silent. I.e. I cannot turn linting on. Edit: Tried on a different file / project. Works. Now to finetuning.

And it now respects ~/.lintr -- much better! Thanks for fixing it.

@jabranham
Copy link
Contributor

jabranham commented Oct 14, 2019 via email

@eddelbuettel
Copy link
Contributor

@jabranham As my follow-ups / edits tried to make clear I think I am good now. I had for a while use a bad combination of lintr settings and R file leading me to concluded it didn't work when it did.

It now seems fine. Still have some issues with Emacs customization for that but I think I will just ignore those and work with a custom ~/.lintr which now works.

Overall still a difficult design issue. "On by default" with all default lintr is a touch or two too aggressive for my tastes. Others in this issue seem to concur.

@jabranham
Copy link
Contributor

Ah, I was reading the email thread and github doesn't notify about edits to posts. Glad to hear you got it working!

@eddelbuettel
Copy link
Contributor

@jabranham To follow-up, I eventually uninstalled that version again as some other things were missing as e.g. the internal help browser. But I am confident it will be straightened out by the next release.

@mweaver-ozette
Copy link

mweaver-ozette commented Oct 18, 2023

ess-use-flymake is a customization variable. If user unsets it, he doesn't want flymake.

Wasted an hour figuring out why flymake keeps running. Tried disabling it with this customization variable, explicitly removing it from hooks, explicitly turning it off in hooks -- yet still flymake insists on running.

Ignoring a customization variable is poor practice.

Suggestion:

diff -u /usr/local/share/emacs/site-lisp/elpa/ess-18.10.2/ess-r-mode.el.\~1\~ /usr/local/share/emacs/site-lisp/elpa/ess-18.10.2/ess-r-mode.el
--- /usr/local/share/emacs/site-lisp/elpa/ess-18.10.2/ess-r-mode.el.~1~	2022-08-29 15:28:31.197022535 -0700
+++ /usr/local/share/emacs/site-lisp/elpa/ess-18.10.2/ess-r-mode.el	2023-10-18 15:30:50.791116142 -0700
@@ -48,7 +48,9 @@
 (when (>= emacs-major-version 25) (require 'ess-r-xref)) ;; Xref API was added in Emacs 25.1
 ;; TODO: Remove when we drop support for Emacs 24:
 (declare-function ess-r-xref-backend "ess-r-xref")
-(when (>= emacs-major-version 26) (require 'ess-r-flymake)) ; Flymake rewrite in Emacs 26
+(when (and ess-use-flymake (>= emacs-major-version 26))
+  (require 'ess-r-flymake)) ; Flymake rewrite in Emacs 26

@mlysy
Copy link

mlysy commented Jan 17, 2024

@mweaver-ozette I also found that (setq ess-use-flymake nil) doesn't work for me (emacs version 28.1 (9.0)). I use flycheck, and succeeded in disabling syntax checking for R with it using

(setq-default flycheck-disabled-checkers '(r-lintr))

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

No branches or pull requests

9 participants