Rather than hooking find-file-hook, define global-rinari-mode

[purcell]

I propose to merge this change shortly, and would welcome feedback from @dgutov, @rejeep, @bbatsov.

[rejeep]

Hey,

I like the change. But I don't really understand the logic in the "maybe" function. Take a look at this commit: https://github.com/rejeep/rinari/commit/1c1ec65c7c91ba75b98e4a313cb1c2a6ba2e86eb

... or am I missing something?

[dgutov]

... or am I missing something?

As I understand it, the idea is to ignore the respective -major-modes variable when its value is nil, so that by default rinari launches in all major modes, like before this change. So for the second test to work, you need to add at least some mode to rinari-major-modes. This may need to be explicitly documented.

I like the change, but I'm not sure about the warning message at the end. If it's going to autoloads, it will be displayed at every startup whether I add an explicit call to the global-mode function in the init file or not. Maybe just update the docs?

By the way, with the message form present, I think the actual call to the global-mode function after it won't be added to autoloads, it needs a progn.

[purcell]

Thanks so much, guys.

@dgutov, yes, you're right about the intended behaviour. Thanks @rejeep for pointing out the lack of clarity.

Regarding the warning message, I kinda wanted to make the change highly visible for a while, until users had adjusted their configs. But simply documenting it might be better. And good catch re. the progn... I'd better fix that!

[dgutov]

Speaking of docs, do you have access to the rubyforge site?

The installation procedure there can use an update. Even if this repo didn't have the link to it in the description, just the existence of it may add to user confusion.

[bbatsov]

@purcell Apart from what @dgutov and @rejeep already mentioned the PR seems good to me.

[purcell]

@dgutov Yes, I have access to the rubyforge site, but I haven't exactly been sure how to approach updating it. The docs are indeed getting quite outdated. I haven't looked at how they were generated. My preferred solution would be to put the info on wiki pages here (or simply in the README), and then redirect the old docs across to github. That requires going through the existing docs, and I haven't invested time in that yet.

[purcell]

Great - I've made a few updates, and merged the changes to master. Let me know if you experience any problems.

[dgutov]

It works well enough, but the warning and the global mode are displayed/enabled twice. So, looking forward to the "future versions" change. :)

[purcell]

@dgutov Can you think of a workaround for that? I'm keen to remove the message, but it'd be nice to make the documentation match up first.

[dgutov]

Replace progn with unless <some variable> and set that variable at the end of the unless form? That defvar would also have to autoloaded.

But that's rather convoluted. If you open the wiki here, I can try to bring over some initial docs.

[dgutov]

Oh, something simpler (haven't tested, should work):

;;;###autoload
(unless (boundp 'rinari-major-modes)
  (message ...)
  (global-...))

[purcell]

I don't have the appropriate permissions on this repo, because I'm not the owner. In the last couple of days I've been considering moving rinari's canonical home to my own account, which I'm pretty sure would be fine with @eschulte.

[purcell]

Or perhaps even:

;;;###autoload
(unless (featurep 'rinari-autoloads)
  (message "Warning: Calling global-rinari-mode automatically for you. Future versions of rinari will require you to do this yourself.")
  (global-rinari-mode))

Since the condition doesn't become true until the autoloads file has been fully evaluated.

[dgutov]

Yep, this is better than my "simple" solution: it doesn't break when the user doesn't install from ELPA.

[dgutov]

Eh, it doesn't help. The call to (global-rinari-mode) in rinari-autoloads file loads rinari before the former has stopped loading, so we still see the message the second time.

[purcell]

LOL! I've committed what I think is a better (and more obvious!) fix. :-)

[dgutov]

So, at this point you probably won't be surprised to know that it doesn't help either. :) rinari.el gets autoloaded before the actual global-rinari-mode definition is called, hence the the mode variable value is nil both times.