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

Need for additional style guide - Use of free variables, unknown functions #69

Open
jeastman opened this issue Feb 20, 2022 · 3 comments
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed

Comments

@jeastman
Copy link
Collaborator

I believe we are need of an additional style guide specific to Rational Emacs, which builds upon The Emacs Lisp Style Guide.

One item in particular is the use of dynamically bound variables (warned by the compiler as free-variable). Since we are forcing lexical binding (with the -*- lexical-binding: t; -*- directive), the use of dynamically bound variables would actually be considered an error as opposed to a warning. Since this is typically does not effect the end result (Emacs still works), I consider it more of a style issue than anything else.

The solution would be to use (defvar somevar) any place a dynamically bound variable (named somevar) is used.

An example from early-init.el would be:

(when (featurep 'native-compile)
  ;; Silence compiler warnings as they can be pretty disruptive
  (setq native-comp-async-report-warnings-errors nil)
  ...)

Where native-comp-async-report-warings-errors is dynamically bound.

I'm suggesting the adoption of a style where one would do the following:

(when (featurep 'native-compile)
  ;; Silence compiler warnings as they can be pretty disruptive
  (defvar native-comp-async-report-warnings-errors)
  (setq native-comp-async-report-warnings-errors nil)
  ...)

Another area where I think it would be proper to adopt a style is in the ensuring functions are declared in the context they are used. This could be handled by ensuring the appropriate module is required in the context, or providing a declare-function statement before the function is used if it could be assumed to compiler would know about it ahead of time.

An example here would be the use of straight-use-package. Assuming that we expect the Rational Emacs initialization to have occurred (which ensures the straight module is available), either a 'require' should be added:

(require 'straight)
...
(straight-use-package 'something)
....

Or the 'declare-function' could be used:

(declare-function straight-use-package "straight" (MELPA-STYLE-RECIPE &optional NO-CLONE NO-BUILD CAUSE INTERACTIVE))
...
(straight-use-package 'something)
...

I understand that Emacs is robust enough for this to "not matter", in the sense that "things will work" without this overhead. However there appears to be a mix of styles with regards to this and I think it would make sense for the project to define the style which should be used in order to provide some consistency.

@bkaestner
Copy link
Contributor

See also #59

@ajxn
Copy link
Contributor

ajxn commented Feb 26, 2022

If it should be a learning experience, we should probably try to add use best practice, so I think these are good suggestions. Add something to the README.org?
Which then could be exported to an info file so the documentation is easy to find from within Emacs.

@jeastman jeastman added the documentation Improvements or additions to documentation label Mar 13, 2022
@jeffbowman jeffbowman added good first issue Good for newcomers help wanted Extra attention is needed labels Jun 27, 2022
@jeffbowman
Copy link
Contributor

See also: #337 #347

Maybe there are other issues related to this one which can be conflated into a single issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants