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

Fix autoloaded register-definition-prefixes #24

Merged
merged 2 commits into from
Nov 26, 2023

Conversation

josephmturner
Copy link
Contributor

Currently, autoload generation with shorthands results in:

(register-definition-prefixes "breadcrumb" '("bc-"))

The autoload prefix should be "breadcrumb-" so that breadcrumb.el will be autoloaded when users type in

M-x describe-function RET breadcrumb-

See (info "(elisp)Autoload by Prefix") for more information.

Related issues:

breadcrumb.el Outdated
@@ -436,6 +436,7 @@ propertized crumbs."
(bc--goto (selected-window) choice))))

(provide 'breadcrumb)
;;;###autoload (register-definition-prefixes "breadcrumb" '("breadcrumb-"))
;;; breadcrumb.el ends here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side-note, if you place the local variables below it, then the statement in this line is no longer accurate. 😬

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to fix. Always found that "ends here" kinda silly anyway. What's the point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to fix. Always found that "ends here" kinda silly anyway. What's the point?

https://www.gnu.org/software/emacs/manual/html_node/elisp/Library-Headers.html says "Its purpose is to enable people to detect truncated versions of the file from the lack of a footer line."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone ahead and fixed it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone ahead and fixed it.

But not in this PR, please. SOrry I should have been clearer.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Its purpose is to enable people to detect truncated versions of the file from the lack of a footer line."

By the way, "enables people". Particular kinds of people who trade in truncated files coming up with genius ideas that no other language have.

@joaotavora
Copy link
Owner

Just so I understand this fix, is this enough to prevent (register-definition-prefixes "breadcrumb" '("bc-")) from happening?

@joaotavora joaotavora reopened this Nov 21, 2023
@joaotavora
Copy link
Owner

Sorry, I closed this from a commit message, but I meant to close #23 instead. So reopening

@tarsius
Copy link
Contributor

tarsius commented Nov 22, 2023

Just so I understand this fix, is this enough to prevent (register-definition-prefixes "breadcrumb" '("bc-")) from happening?

No, it only ensures that the correct prefix is also registered.

@joaotavora
Copy link
Owner

And are there any adverse effects (that you know of) to having two prefixes registered for a given package?

@tarsius
Copy link
Contributor

tarsius commented Nov 23, 2023

I'm not aware of any, but I know very little about all this, so that doesn't mean much.

@joaotavora
Copy link
Owner

I think

;; Local Variables:
;; autoload-compute-prefixes: nil
;; End:

Should be added to this PR.

Also please make the commit message in the GNU changelog format (like other commit messages in this repo).

@josephmturner
Copy link
Contributor Author

And are there any adverse effects (that you know of) to having two prefixes registered for a given package?

I don't know, but I agree with you that bc- should not be a registered prefix.

;; Local Variables:
;; autoload-compute-prefixes: nil
;; End:

I tried this, but as you pointed out in your email to the mailing list, autoload-compute-prefixes is not a variable, let alone a safe variable. Do you still want to add a local variable declaration even though Emacs will prompt about variables that may not be safe?

Also please make the commit message in the GNU changelog format (like other commit messages in this repo).

Done.

@joaotavora
Copy link
Owner

Do you still want to add a local variable declaration even though Emacs will prompt about variables that may not be safe?

I want this pull request fix register-definition-prefixes (whatever their point is) so that breadcrumb- is registered, and bc- is NOT registered.

josephmturner and others added 2 commits November 26, 2023 14:03
* breadcrumb.el: Add correct register-definition-prefixes autoload so
that "breadcrumb-" is registered as a prefix.  Locally set
autoload-compute-prefixes to nil so that "bc-" is not registered.

Co-authored-by: Jonas Bernoulli <[email protected]>
@josephmturner
Copy link
Contributor Author

I want this pull request fix register-definition-prefixes (whatever their point is) so that breadcrumb- is registered, and bc- is NOT registered.

Done! Emacs master branch master now autoloads (put 'autoload-compute-prefixes 'safe-local-variable), so my previous concern is resolved!

Thanks!

@joaotavora joaotavora merged commit dcb6e2e into joaotavora:master Nov 26, 2023
@joaotavora
Copy link
Owner

OK merged. And thanks. I'd rather the "ends here" thing hadn't been touched here, but no biggie anyway, I'll just remove that silly trailer soon anyway.

@tarsius
Copy link
Contributor

tarsius commented Nov 27, 2023

OK merged. And thanks. I'd rather the "ends here" thing hadn't been touched here, but no biggie anyway, I'll just remove that silly trailer soon anyway.

I agree that it is silly in the age of version-control and would like to remove it from my libraries. But it's still the convention and if you go against it, linters will tell you about it.

I have learned to live with it and it is certainly not the hill I want to die on, but if you feel more strongly about it, I would encourage you to try to get this convention removed.

Or maybe just relaxed to IFF the file does not end with (provide 'the-feature), then it must end with ;;; the-feature.el ends here, or (at your option) ;; the-feature.el ends here (to make it less annoying for people using outline-minor-mode).

@josephmturner
Copy link
Contributor Author

Or maybe just relaxed to IFF the file does not end with (provide 'the-feature), then it must end with ;;; the-feature.el ends here, or (at your option) ;; the-feature.el ends here (to make it less annoying for people using outline-minor-mode).

In that case, in breadcrumb.el, (provide 'breadcrumb) would need to go after:

;;;###autoload (register-definition-prefixes "breadcrumb" '("breadcrumb-"))

;; Local Variables:
;; read-symbol-shorthands: (("bc-" . "breadcrumb-"))
;; autoload-compute-prefixes: nil
;; End:

@joaotavora
Copy link
Owner

joaotavora commented Nov 29, 2023

But it's still the convention and if you go against it, linters will tell you about it.

So what?

I would encourage you to try to get this convention removed.

Why me? you can try too! :-)

All these text-based tools are brittle and they don't cooperate. For example, the real reason that the "ends here" thing happened before the actual end is because add-file-local-variable, which is what I use to add file-local variables, doesn't know about it.

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

Successfully merging this pull request may close these issues.

3 participants