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

Suggestion: use a minor to enable auto export #247

Closed
edkolev opened this issue Dec 23, 2018 · 4 comments
Closed

Suggestion: use a minor to enable auto export #247

edkolev opened this issue Dec 23, 2018 · 4 comments

Comments

@edkolev
Copy link
Contributor

edkolev commented Dec 23, 2018

Thank you for this package, it really has helped me blog with ease!

I would like to suggest the following behaviour for enabling auto export - define a minor mode ox-hugo-auto-export-mode.

This has the following benefits:

  1. it's more in-line with emacs standards, minor modes are a perfect fit for toggling functionality
  2. having the following will be enough to enable auto export (no (require 'ox-hugo-auto-export) needed):
(("content-org/"
  (org-mode
   (eval . (org-hugo-auto-export-mode)))))
  1. to disable/enable, the mode can be simply toggled; now, changing a local variable requires writing lisp in the minibuffer
  2. it would be possible to enable the auto export through .dir-locals.el; now, I have to write some lisp (require 'ox-hugo-auto-export) in my scratch buffer, and only then go to my .org file; if I forget to run the require before opening the .org file, I have to require and then revert my .org buffer
  3. the after-save-hook will not be polluted for all the org files the user opens
  4. a minor mode can be easily autoload-ed; no code will be loaded until the user enables the mode

In short, it all boils down to this - it's more idiomatic to use a minor mode in this.

I have a working prototype here https://github.com/edkolev/ox-hugo/blob/use-minor-mode-to-toggle-auto-export/ox-hugo-auto-export-mode.el, I'm open to adjusting it as you see fit and opening a PR. I've signed the FSF copyright agreement.

In case you agree this is the right direction, care about backwards compatibility should be taken. Maybe the ox-hugo-auto-export.el file could be deprecated, but not removed. If anyone requires it, a warning would be printed. Or something along these lines.

Sorry for ignoring the issue template, this is a suggestion rather than a bug report.

Thanks for considering this!

@kaushalmodi
Copy link
Owner

Thank you for this package, it really has helped me blog with ease!

You are welcome!

I would like to suggest the following behaviour for enabling auto export - define a minor mode ox-hugo-auto-export-mode.

I am completely for it! See details below.

This has the following benefits:

(("content-org/"
 (org-mode
  (eval . (org-hugo-auto-export-mode)))))

A better way would be (mode . org-hugo-auto-export) instead of (eval . (org-hugo-auto-export-mode)). See this example of enabling auto-fill-mode.

In short, it all boils down to this - it's more idiomatic to use a minor mode in this.

I agree completely.

I have a working prototype here https://github.com/edkolev/ox-hugo/blob/use-minor-mode-to-toggle-auto-export/ox-hugo-auto-export-mode.el, I'm open to adjusting it as you see fit

edkolev@00ba9e5 looks great! Thanks for working on this.

and opening a PR. I've signed the FSF copyright agreement.

Awesome! 👍

Please open a PR. We can work on the fine-tuning of the commit there.

Major point: Please update the docs too (this section).
`

In case you agree this is the right direction, care about backwards compatibility should be taken. Maybe the ox-hugo-auto-export.el file could be deprecated, but not removed.

If anyone requires it, a warning would be printed. Or something along these lines.

Yes. I am planning on adding a warning in that file and then removing it in the next month. But I can take care of that. In your PR, git move (rename) the ox-hugo-auto-export.el to ox-hugo-auto-export-mode.el first and then in a separate commit revive the old ox-hugo-auto-export.el using a regular copy --- I'd like to retain the commit history of ox-hugo-auto-export.el in the new ox-hugo-auto-export-mode.el. I hope that makes sense. Feel free to clarify.

Sorry for ignoring the issue template, this is a suggestion rather than a bug report.

No worries.

Thanks for considering this!

I am always up for doing the Right Thing. I was aware that minor modes are the right way to go, but somehow it didn't occur to me when I did a recent refactoring for this auto-export feature. So thank you for bringing this up.

@edkolev
Copy link
Contributor Author

edkolev commented Dec 24, 2018

A better way would be (mode . org-hugo-auto-export) instead of (eval . (org-hugo-auto-export-mode)). See this example of enabling auto-fill-mode.

Nice, I didn't know this worked - I though it would change the major mode like ;; Local Variables: would as noted here

I just submitted the PR.

@kaushalmodi
Copy link
Owner

@edkolev

Hello! Many thanks for bringing up this issue and doing most of the groundwork in your PR.

Everything is now committed to master.

You can find the details here:

Let me know if this issue can be closed (or you can do that too).

@edkolev
Copy link
Contributor Author

edkolev commented Jan 4, 2019

Nice, thanks for working on this awesome project and making it even better!

@edkolev edkolev closed this as completed Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants