docs(NvimClient.API): add docstrings for public API #15

smolck · 2021-09-08T22:25:09Z

Just a start for now, probably will want to update the generator to generate the docstrings for the public event EventHandler ...s to fully document everything (and silence the warnings when publishing the package).

src/NvimClient.API/NvimAPI.cs

Co-authored-by: Justin M. Keyes <[email protected]>

lgtm-com · 2021-09-09T05:35:17Z

This pull request introduces 1 alert when merging 9220e9a into a66c3b2 - view on LGTM.com

new alerts:

1 for Use of default ToString()

smolck · 2021-09-09T05:37:51Z

Okay so I've added docs (rough draft) for the UI events; turns out the ui.txt docs for the ui events aren't easily available for use, and I'm not sure if it'd even be worth all the extra work to try and get them (see this commit message).

Ultimately the docs are available (in an arguably better place, for navigation at least) via a quick :help ui-events in nvim, so it's not totally necessary to add them, though I guess it could be nice to have? The thing is it sort of adds noise where as a user of the library you quite possibly just want to know how to use the handler(s), and will read up in the actual nvim help docs if you want to know what the events do/mean (and I mean, if you're dealing with all of this to e.g. make a UI, you should probably read/be reading ui.txt anyways).

Thoughts?

smolck · 2021-09-09T05:40:10Z

src/NvimClient.APIGenerator/NvimAPIGenerator.cs

+    /// // the above handler code will be run.
+    /// </code>
+    /// </example>
+    /// <seealso cref=""{camelCaseName}EventHandler""/>


This is just what I came up with relatively spur-of-the-moment for the docs, lmk if/how it can be improved (and assuming this is how we want to do the docs for these EventHandlers).

I think we'd need to parse the ui.txt help doc if we wanted to get the documentation from there and use it here, and after testing how it'd look with the grid_line event docs, I'm not even convinced it'd be a good idea anyways to use the docs from ui.txt

justinmk · 2021-09-09T07:41:05Z

turns out the ui.txt docs for the ui events aren't easily available for use, and I'm not sure if it'd even be worth all the extra work to try and get them (see this commit message).

yeah I wouldn't bother with that; rather, ui.txt itself should be generated from code, then we could expose it in nvim_get_api().

Thoughts?

I'd say wait until/unless nvim_get_api() exposes the ui.txt docs in a structured way.

justinmk · 2021-09-09T07:45:33Z

src/NvimClient.APIGenerator/NvimAPIGenerator.cs

@@ -123,11 +123,28 @@ private object GetExtensionType(MessagePackExtendedTypeObject msgPackExtObj)
      string.Join("\n",
        uiEvents.Select(uiEvent =>
        {
-          var camelCaseName = StringUtil.ConvertToCamelCase(uiEvent.Name, true);
+          var camelCaseName = $"{StringUtil.ConvertToCamelCase(uiEvent.Name, true)}Event";


Suggested change

var camelCaseName = $"{StringUtil.ConvertToCamelCase(uiEvent.Name, true)}Event";

var camelCaseName = $"{StringUtil.ConvertToCamelCase(uiEvent.Name, true)}UIEvent";

or...

Suggested change

var camelCaseName = $"{StringUtil.ConvertToCamelCase(uiEvent.Name, true)}Event";

var camelCaseName = $"UI{StringUtil.ConvertToCamelCase(uiEvent.Name, true)}Event";

Note that if I do this I’ll have to modify a few other parts of the code, since this changes the name structure of all the events entirely (from what #11 made them).

I’m fine with doing this if you think it’s a good idea (although there’s something I dislike about all the events having UIEvent at the end of their names, when we could just do api.UIEvents.Put etc. instead), but it’s kinda outside the scope of this PR. Maybe better suited to a follow-up one?

justinmk · 2021-09-09T07:48:49Z

src/NvimClient.APIGenerator/NvimAPIGenerator.cs

+    /// // Now if this event is emitted after attaching the UI,
+    /// // the above handler code will be run.


maybe move the comment above the handler definition, and...

Suggested change

/// // Now if this event is emitted after attaching the UI,

/// // the above handler code will be run.

/// // This handler will be executed whenever the event is emitted after attaching the UI.

👍 just unresolved this until I move the comment, then I’ll merge.

Co-authored-by: Justin M. Keyes <[email protected]>

smolck · 2021-09-10T19:27:00Z

Okay so some of the rest of the documentation-related warnings, specifically those for certain params not being documented, I'm not quite sure why the docs aren't being generated to fix those. Also, the reason UiTryResize etc. don't have documentation is because they aren't documented in the neovim source.

I don't think any of those things need to block this (and thus a NuGet release); any objections to just going ahead and merging this, putting out a 0.2 nuget release, and then getting the rest of the docs situation sorted out after? Probably starting upstream with adding documentation for the UI events in the source (in ui_events.in.h) and generating ui.txt from that . . .

justinmk · 2021-09-10T21:37:27Z

🚢 🇮🇹

smolck commented Sep 8, 2021

View reviewed changes

src/NvimClient.API/NvimAPI.cs Show resolved Hide resolved

docs(): add docstrings for NvimAPI.cs public members

ccbc026

smolck force-pushed the document-public-api branch from eb726ea to ccbc026 Compare September 8, 2021 22:26

justinmk reviewed Sep 8, 2021

View reviewed changes

src/NvimClient.API/NvimAPI.cs Outdated Show resolved Hide resolved

better doc for NvimAPI class (suggestion)

c0d992b

Co-authored-by: Justin M. Keyes <[email protected]>

smolck force-pushed the document-public-api branch from 7eac95e to 9220e9a Compare September 9, 2021 05:28

smolck commented Sep 9, 2021

View reviewed changes

smolck force-pushed the document-public-api branch 4 times, most recently from d88b648 to b4c0f1c Compare September 9, 2021 05:48

add docs for UI Events

ccd2660

I think we'd need to parse the ui.txt help doc if we wanted to get the documentation from there and use it here, and after testing how it'd look with the grid_line event docs, I'm not even convinced it'd be a good idea anyways to use the docs from ui.txt

smolck force-pushed the document-public-api branch from b4c0f1c to ccd2660 Compare September 9, 2021 05:59

justinmk reviewed Sep 9, 2021

View reviewed changes

smolck and others added 2 commits September 9, 2021 09:32

improve comment (suggestion)

a57654d

Co-authored-by: Justin M. Keyes <[email protected]>

update generated api

9a22c0a

smolck marked this pull request as ready for review September 10, 2021 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(NvimClient.API): add docstrings for public API #15

docs(NvimClient.API): add docstrings for public API #15

smolck commented Sep 8, 2021

lgtm-com bot commented Sep 9, 2021

smolck commented Sep 9, 2021 •

edited

Loading

smolck Sep 9, 2021

justinmk commented Sep 9, 2021

justinmk Sep 9, 2021

smolck Sep 9, 2021

justinmk Sep 9, 2021

smolck Sep 11, 2021

smolck commented Sep 10, 2021

justinmk commented Sep 10, 2021

	var camelCaseName = $"{StringUtil.ConvertToCamelCase(uiEvent.Name, true)}Event";
	var camelCaseName = $"{StringUtil.ConvertToCamelCase(uiEvent.Name, true)}UIEvent";

	var camelCaseName = $"{StringUtil.ConvertToCamelCase(uiEvent.Name, true)}Event";
	var camelCaseName = $"UI{StringUtil.ConvertToCamelCase(uiEvent.Name, true)}Event";

		/// // Now if this event is emitted after attaching the UI,
		/// // the above handler code will be run.

	/// // Now if this event is emitted after attaching the UI,
	/// // the above handler code will be run.
	/// // This handler will be executed whenever the event is emitted after attaching the UI.

docs(NvimClient.API): add docstrings for public API #15

Are you sure you want to change the base?

docs(NvimClient.API): add docstrings for public API #15

Conversation

smolck commented Sep 8, 2021

lgtm-com bot commented Sep 9, 2021

smolck commented Sep 9, 2021 • edited Loading

smolck Sep 9, 2021

Choose a reason for hiding this comment

justinmk commented Sep 9, 2021

justinmk Sep 9, 2021

Choose a reason for hiding this comment

smolck Sep 9, 2021

Choose a reason for hiding this comment

justinmk Sep 9, 2021

Choose a reason for hiding this comment

smolck Sep 11, 2021

Choose a reason for hiding this comment

smolck commented Sep 10, 2021

justinmk commented Sep 10, 2021

smolck commented Sep 9, 2021 •

edited

Loading