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

recipes/general-close added #2958

Merged
merged 1 commit into from Jul 25, 2015
Merged

recipes/general-close added #2958

merged 1 commit into from Jul 25, 2015

Conversation

andreas-roehler
Copy link
Contributor

No description provided.

@andreas-roehler
Copy link
Contributor Author

melpa was requested several times

See emacs-berlin/syntactic-close#2

@purcell
Copy link
Member

purcell commented Jul 24, 2015

Thanks. The symbol naming is a bit random in these files. There are prefixes like gen--, gen-, gc-- etc. -- the code should instead follow the elisp coding conventions and prefix all top-level symbols with general-close- (or general-close-- for package-private symbols).

@purcell purcell added recipes awaiting-upstream Awaiting action from an upstream maintainer labels Jul 24, 2015
@andreas-roehler
Copy link
Contributor Author

Am 24.07.2015 um 14:41 schrieb Steve Purcell:

Thanks. The symbol naming is a bit random in these files. There are
prefixes like |gen--|, |gen-|, |gc--| etc. -- the code should instead
follow the elisp coding conventions
https://www.gnu.org/software/emacs/manual/html_node/elisp/Coding-Conventions.html
and prefix all top-level symbols with |general-close-| (or
|general-close--| for package-private symbols).


Reply to this email directly or view it on GitHub
#2958 (comment).

Done, thanks!

@purcell
Copy link
Member

purcell commented Jul 24, 2015

There's also some code which is creating and setting unprefixed top-level variables, e.g. (setq done (member (char-before) (list ?\t ?\n ?\ ?=))) here. You should be using (let ...) to set values locally for a block like that, and if you need to refer to them elsewhere (as you appear to here) then they should be declared as proper top-level defvars with the full package prefix.

@andreas-roehler
Copy link
Contributor Author

Am 25.07.2015 um 00:42 schrieb Steve Purcell:

There's also some code which is creating and setting unprefixed
top-level variables,

Not really.

`done' is not a top-level variable, but let-bound by the top-level command.
Settings here should effect environment, can't introduce a local let.

e.g. |(setq done (member (char-before) (list ?\t ?\n ?\ ?=)))| here
https://github.com/emacs-berlin/general-close/blob/master/general-close-modes.el#L74.
You should be using |(let ...)| to set values locally for a block like
that, and if you need to refer to them elsewhere (as you appear to
here
https://github.com/emacs-berlin/general-close/blob/master/general-close-modes.el#L83)
then they should be declared as proper top-level |defvar|s with the
full package prefix.


Reply to this email directly or view it on GitHub
#2958 (comment).

The value is expected to be bound inside a let, it should not conflict
with parellel runs of general-close.

Can't savely use defvar AFAIU.

@purcell
Copy link
Member

purcell commented Jul 25, 2015

The value is expected to be bound inside a let, it should not conflict
with parellel runs of general-close.

Eww, right, I didn't notice that it was a let-bound parameter implicitly passed from the other file.

purcell added a commit that referenced this pull request Jul 25, 2015
@purcell purcell merged commit 641a48f into melpa:master Jul 25, 2015
@andreas-roehler
Copy link
Contributor Author

Am 25.07.2015 um 08:55 schrieb Steve Purcell:

Merged #2958 #2958.


Reply to this email directly or view it on GitHub
#2958 (comment).

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-upstream Awaiting action from an upstream maintainer recipes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants