File midi.mli documentation #11
Conversation
|
Mention the Issue solved by this PR, easier to refer to it then! |
Okay, I'll do that. Thanks! |
There was a problem hiding this comment.
Hello @Mojoeffect, thanks for your contribution to the project!
Here is a first review for you, I will go back to it, this was merely a first pass. 😊
bin/midi.mli
Outdated
|
|
||
| val error_to_string : Portmidi.Portmidi_error.t -> string | ||
| (** [error_to_string] returns [Portmidi_error] message if initializing the portmidi library fails *) |
There was a problem hiding this comment.
error_to_string is more like. a convenience function to transform a Portmidi_error.t into a string, it does not return an error per se
There was a problem hiding this comment.
Ohhh, that actually sounds way better!
bin/midi.mli
Outdated
|
|
||
| module Device : | ||
| (** [Device] entails the implementation of the [output_device] *) |
There was a problem hiding this comment.
Device is more intended as a wrapper around output_device.
It could be described as a module representing a MIDI device we can send MIDI data to. 😊
There was a problem hiding this comment.
Duly noted. So can one say the Device module is a representation of the overall implementation of the MIDI device?
bin/midi.mli
Outdated
| sig | ||
| type t | ||
| (** [t] is a [Device type] having the [device_id] and [device] parameters. |
There was a problem hiding this comment.
In OCaml, when in the type signature you write, can you omit the full definition. (here see how we only say type t, rather than talking about device, and device_t)
The idea how such definition (said abstract) is to leave out to the user the underlying implementation details: they do not know what comprises this type.
So describing that it contains a device_id and a device is maybe not useful here: the user of the module does not need to know about these. :)
There was a problem hiding this comment.
Guess I went off track in the process of trying to collaborate the midi.ml file with the midi.mli file.
I'll make the necessary changes strictly focusing on the midi.mli file
bin/midi.mli
Outdated
| val create : int -> t | ||
| (** [create] creates and opens MIDI [output_device], returns [device] and [device_id]. |
There was a problem hiding this comment.
If you follow the type signature here, it says int -> t, which means the function only takes an integer and returns a Device.t.
buffer_size, device_id. and latency are parameters we pass to Portmidi in order to open said device: the user of the function create do not know about these, so we can omit mentioning though.
You may want to mention, however, that the int parameter passed as an argument should represent an output MIDI device on the system, that the user can retrieve using the list_device command built in cardio-crumble.
As for the error, you may simply say that on failure, create will exit the program with exit code 1. (it is not good practice, but is the current behaviour :-).)
There was a problem hiding this comment.
Okay, I'll make the necessary changes. I had thought those extra parameters would give an insight to some behind the scene workings, that's why I added them... I know better now, thanks!
bin/midi.mli
Outdated
|
|
||
| val write_output : Device.t -> Portmidi.Portmidi_event.t list -> unit | ||
| (** [write_output] writes a midi_event message to the [output_device]. Returns a unit for |
There was a problem hiding this comment.
I would simply write it like such: "returns unit regardless of if the message was successfully sent or not".
There was a problem hiding this comment.
Okay, I'll edit that right away
There was a problem hiding this comment.
Nice, thanks a lot to both of you @Mojoeffect and @Engil!
I've noticed that I was possibly a bit optimistic when tagging this as good-first-issue. Thanks a lot for taking it on anyways, @Mojoeffect!
Similarly to @Engil, I only have a first comment for now: Let's take the example of function documentation. The way people read documentation is by having a look at both the function signature and the documentation and combining the two to understand the function. So describing the function signature inside the documentation doesn't add new information. Instead, it's better to put the function into context or to explain what the arguments or return value represent. I'm leaving a comment below to explain what I mean by an example.
Let me know if this is helpful or if I should explain more concretely what I mean!
| (** [message_on] creates a portmidi message. Where [volume] may be provided. | ||
| [message_on] requires [note] and [timestamp]. *) |
There was a problem hiding this comment.
This is one of the cases I was mentioining where a higher-level description would be better. There are different possible ways to add information here.
You could explain in which format the note argument is expected. In which way does a char type value represent a musical note? To be honest, that might not at all be obvious, so if you have trouble finding that out, don't hesitate to ask!
You could also put the function in the MIDI context. What kind of MIDI message does it represent? A channel message? A voice message?
Another option is to explain how to use the returned value later: by passing it to write_output.
I'd personally probably add all those pieces of information.
Please let me know if what I'm saying is too abstract! If that would be helpful, I could give you one concrete example of how I'd write the documentation concretely here.
|
Hi @Mojoeffect, I hope our comments haven't been demotivating for you. To be clear: you made a good start! The issue of adding API documentation is simply a bit harder than If there's anything you'd like us to clarify or help with, let us know. |
Current documentation for the midi.mli file.
This resolves #5
Corrections and tips would be appreciated to effect any necessary update or edit of the file.
Thanks :)