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

docs: Improve plugin documentation #3240

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glupi-borna
Copy link

Added direct links to relevant external documentation pages, rewrote some sections, documented missing functions/callbacks, reformatted everything to fit in 80 columns or less. Attempted to optimize for legibility within micro, without sacrificing what the rendered markdown looks like.

This should address some of the stuff that came to light in #3231.

I think the layout of the rendered markdown of the package/callback docs could be greatly improved, but it is hard to do so without hurting the readability within micro.

from the go plugin, which has the following file structure:
provides additional information, such as the name of the plugin, the
plugin's website, dependencies, etc.
[Here is an example `repo.json` file][repo.json] from the go plugin, which has
Copy link
Collaborator

Choose a reason for hiding this comment

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

It took me some time to figure out that this [repo.json] is a reference to the "footnote" below.

As you noted yourself, we need to care about readability of this help inside micro, i.e. as plain text, not just as rendered markdown. So I'm not sure this change is a good idea.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I thought it might not be totally clear what's going on there, but I decided to go with it anyway because I thought the links break the flow and layout of the text a bit.

But that's my bad, I'll get rid of this type of link. Would you prefer I keep the links to the golang documentation in some form (as literal footnotes maybe?), or should I completely get rid of them?

Also, thanks so much for taking the time to review this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you prefer I keep the links to the golang documentation in some form (as literal footnotes maybe?), or should I completely get rid of them?

I'm not against keeping them in some form, I do agree that providing more granular links than just the current Please go to the internal documentation at https://pkg.go.dev/github.com/zyedidia/micro/v2/internal to see the full list of available methods. should make the documentation more convenient to use.

syntax header files. No directory structure is enforced but keeping
runtime files in their own directories is good practice.
cases, a plugin may also want to add colorschemes or syntax files. It
is unlikely for a plugin to need to add plugin or syntax header files at
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this sentence meant to say that a plugin is unlikely to load syntax header files at all. Actually, a plugin is not even able to load them, since RTSyntaxHeader is not exported to Lua, unlike other runtime file types. So I think we should just remove "or syntax header files" here.

Copy link
Author

Choose a reason for hiding this comment

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

Nice catch! I didn't even RTSyntaxHeader is not exported.

initialized.
* `deinit()`:
cleanup function called when your plugin is unloaded, either manually by the
user, or by running the `reload` command.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with changing the style (function description beginning not at the same line but at the next line), but please use consistently the same indentation everywhere.

Also could you move these style changes (that are moving the text without changing its contents) to a separate commit, to separate them from the actual text changes? It's really hard to review when the two are mixed.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, you're right. I'll amend this PR to remove the changes, and then I'll make another one with just the layout/styling changes at some point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just in case, I didn't mean necessarily a separate PR. It can be done in one PR, just in separate commits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With 2b384de it looks good now, but could you squash 2b384de into the previous commit, to make it easy to review? (Actually I already ended up doing that myself locally and reviewing via git difftool. But what if others want to review it too.)

Copy link
Author

Choose a reason for hiding this comment

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

Good call! I was going to squash it but I guess I got distracted and forgot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the delay, didn't realize you already squashed it. The damn github didn't send any notification.

Copy link
Author

Choose a reason for hiding this comment

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

No worries at all! :)


* `postinit()`: initialization function called after `init()`.
* `onSetActive(`[bufpane][BufPane]`)`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, a user reading help plugins in micro (in plain text) will have no idea what the heck is this [bufpane][BufPane].

Copy link
Contributor

Choose a reason for hiding this comment

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

Having the links in the rendered markdown is extremely useful though so maybe the tradeoff is worth it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is not a tradeoff but the opposite of it, i.e. favoring one use case at the expense of the other one.

A tradeoff would be something like: do add these links, but only as "footnotes". Perhaps also with a sentence like See these links for documentation on these data structures:.

Copy link
Author

Choose a reason for hiding this comment

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

Right, the primary use case for the docs is to be viewed from within micro, so we should optimize for that and strive to remove as much visual noise as possible. Maybe the links could be kept somewhere close by, but not within the callback signatures themselves.

stdout from the command to the returned string.

- `JobStart(cmd string, onStdout, onStderr,
onExit func(string, []interface{}), userargs ...interface{})
*exec.Cmd`:
*Cmd`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

Copy link
Author

Choose a reason for hiding this comment

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

After adding links, it seemed to me that the exec. (and possibly buffer. and display.) were just unnecessary noise, but that might just be my perspective. In fact, I think that just dumping go function signatures directly into the plugin docs is not that good of an idea, seeing as plugins are written in Lua. It expects that plugin authors understand at least some subset of go, which is likely not true in a lot of cases.

That said, I decided not to change the layout of all the function signatures because it seemed too big of a change for what I'm trying to achieve here (in retrospect, I should have left the layout alone too and just focused on the content of the docs). I did attempt to do it (for a little while at least), just to explore the possibility space a bit, but I didn't come up with a strictly better layout (in regards to being readable in both micro and in a markdown viewer).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact, I think that just dumping go function signatures directly into the plugin docs is not that good of an idea, seeing as plugins are written in Lua.

But this is not just about docs, it is a deeper "issue". The fact is that although plugin authors write their plugins in Lua, for the most part they use APIs that are defined in Go, not in Lua. Just because that's how Micro's plugin system is designed, for good or bad. So the docs should not pretend that it's not like that, I think.

It expects that plugin authors understand at least some subset of go, which is likely not true in a lot of cases.

But the reality is that they do need to understand at least some subset of go (see above). At least to understand signatures of Buffer or BufPane methods, and so on.

There are 7 default plugins that come pre-installed with micro. These are

* `autoclose`:
automatically closes brackets, quotes, etc...
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that moving these to separate lines here really makes it more readable.

Copy link
Author

Choose a reason for hiding this comment

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

I think I moved them because for consistency with the function descriptions. Now that I look at it, it really does not improve anything.

Added direct links to relevant external documentation pages, rewrote
some sections, documented missing functions/callbacks, etc.
@@ -51,6 +50,9 @@ which micro defines:

* `postinit()`: initialization function called after `init()`.

* `deinit()`: cleanup function called when your plugin is unloaded,
either manually by the user, or by running the `reload` command.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not clear what does it mean "unloading manually" (I ended up looking into the code to recall what it means).

So, it means "disabling plugin via the set command", right?

BTW, the reload command is also run by the user, and also manually, right? :)

BTW preinit(), init() and postinit() are also called when running the reload command. It doesn't mean that we need to mention it in their above descriptions as well, but we should probably better document the reload command itself, in help commands. Currently it is:

* `reload`: reloads all runtime files.

It's likely not clear to the user what is "runtime" files. So maybe let's make it clear:

* `reload`: reloads all runtime files (settings, keybindings, syntax files,
   colorschemes, plugins).

And then maybe also document its behavior wrt callbacks, something like:

* `reload`: reloads all runtime files (settings, keybindings, syntax files,
   colorschemes, plugins). For plugins, it properly unloads already loaded
   plugins (by running their `deinit()` callbacksif there are any) and then
   re-initializes reloaded plugins (by running their `preinit()`, `init()`
   and `postinit()` callbacks if there are any).

Please feel free to suggest a better wording.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, do we document anywhere that the set command can be used to enable/disable plugins, not just options? Looks like we don't. (I thought we did...) So it makes sense to document it (both in help plugins and in the description of the set command in help commands)?

Copy link
Author

Choose a reason for hiding this comment

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

So, it means "disabling plugin via the set command", right?

Correct, and yeah, good point about documenting set and reload better.

I mean help options does say that set can be used to enable/disable plugins, but it's only obvious if you read it immediately after reading about set in help commands:

help commands

[...]

  • set 'option' 'value': sets the option to value. See the options help topic for a list of options you can set. This will modify your settings.json with the new value.

[...]

help options

[...]
Plugin options: all plugins come with a special option to enable or disable
them. The option is a boolean with the same name as the plugin itself.
[...]

However, this is super unclear and roundabout, and might confuse new users. It would definitely be much better to have the behavior explicitly documented. I'll give it a shot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

help options

[...]
Plugin options: all plugins come with a special option to enable or disable
them. The option is a boolean with the same name as the plugin itself.
[...]

Ah, indeed. So it's actually documented, so it's not so bad. So maybe it's not necessary to lengthen the description of the set command with an explicit mentioning of plugin options. Still, I guess we should add mentioning them at least to help plugins, since it is, well, about plugins. (I.e. help plugins is probably the place where users would look for info how to disable a plugin and so on.)

@@ -51,6 +50,9 @@ which micro defines:

* `postinit()`: initialization function called after `init()`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

While we're at it, shouldn't we improve this description: clarify that it is called after the init() callbacks of all other plugins have been called? Otherwise it's not clear what's the point of this postinit() callback.

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely, makes sense.

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.

None yet

3 participants