Have to chase around in code to figure out what parameters a command takes

[njx]

Currently, Commands.js lists all the commands you can call, but in order to figure out what arguments a command expects, you have to hunt for the object that handles it, find what method of that object gets called, and read the documentation there. We should probably add the documentation directly in comments in Commands.js.

[pthiess]

Reviewed assigned to NJ

[lkcampbell]

@njx,

I would like to fix this bug.

Did you have any specific format in mind for the documentation comments? If you do, please give me an example of one and I will use that format to comment all of the commands. If not, I will come up with one on my own.

Either way, let me know.

[lkcampbell]

@njx, I took a shot at documenting a section. Attached are screenshots of a couple of possibilities. I personally like the second one better. Note that I am not documenting the parameters. I simply provide the file name and the handler. With Quick Open, and these two pieces of information, you can find the parameter information fairly quickly in the original file, without filling Commands.js with a lot of comments. Let me know what you think:

Handler function listed first:

File name listed first:

[TomMalbran]

Nice. I think the second one is better, since you need to first go to the file and then to the function.

It would be awesome if Quick Open would handle queries like file@function so we can then use those here, and then selecting and using quick open would take you right to the function.

[lkcampbell]

@TomMalbran, agreed. That would be a nice shorthand reference to a module-level function. Ideally, Quick Open would find it dynamically, but even if it didn't, it could still navigate to the file and make an attempt to find the function when it got there. I think we are thinking of a format like:

ViewCommandHandlers@_handleIncreaseFontSize

or any other strings that are reasonably close to that. It's fairly concise and can be included in comments without filling up too much space.

@njx, that's two votes for the second one. If you can take a moment to review and let me know what you think, I can finish up this documentation this week.

[njx]

I like it. 👍

[redmunds]

From @peterflynn (in #5155): "This seems like it will be hard to keep in sync with the actual code -- I'm guessing it will drift out of date fairly quickly. I wonder if it's better to only list the module name? That probably changes less often than the function name, and once you have the module name it's trivial to find the command handler within it (that is, telling you the module name is the part with the most bang for the buck)."

[redmunds]

I don't think existing commands will change that much, but we'll definitely need to update this for new commands. Anyone adding a new command will have to update this file, so they'll see the format. Between submitter and reviewer, I think we should be able to keep this reasonably up-to-date.

[lkcampbell]

I considered that approach as well. It is cleaner and still addresses the main problem, which is identifying handler's module without resorting to Find in Files. I opted for the more inclusive solution first but I would consider either solution to be adequate.

@njx, what are your thoughts on this? Should we include the handler function names or get rid of them?

[njx]

I don't think we change existing handlers that often, which is why I thought it was okay to go down this route. If it looks like it's too much of a maintenance headache in the future, we can remove them later.

[redmunds]

Closing.