repl: Undocumented public methods on REPLServer.prototype

[lance]

There are a number of methods on REPLServer.prototype that are clearly available in userland but are not documented. In at least a few cases, my impression is that these methods are attached to the prototype primarily in order to facilitate testing. I think this situation is non-ideal.

• Version: v6.2.2
• Platform: Darwin Lances-MacBook-Pro.local 15.5.0 Darwin Kernel Version 15.5.0: Tue Apr 19 18:36:36 PDT 2016; root:xnu-3248.50.21~8/RELEASE_X86_64 x86_64
• Subsystem: repl, doc

I have seen similar discussions as a part of various issues regarding "semi-private" properties that begin with an underscore on various other modules - though typically those conversations are tangential to the real issue.

This issue is similar - publicly accessible but undocumented properties. The pattern is not limited to REPLServer, but since this is where I have spent most of my time in the code base, it's the one I'm most familiar with now, so for now, this conversation is limited to that module. However, I do think this should be discussed at a higher level and some guidance created for dealing with these "leaks".

As a rule, I would say that no methods should be attached to an object's prototype that are undocumented. But it's not clear how to best deal with existing concerns.

Here is a list of the methods found on REPLServer.prototype that are currently undocumented, but are fully accessible in userland.

REPLServer.prototype.close // inherited from readline.Interface
REPLServer.prototype.setPrompt // inherited from readline.Interface
REPLServer.prototype.createContext
REPLServer.prototype.resetContext
REPLServer.prototype.complete
REPLServer.prototype.parseREPLKeyword
REPLServer.prototype.memory
REPLServer.prototype.convertToContext

I can see why close and setPrompt may not be documented. Although, I think ideally some mention of them should be made in the REPL context since REPLServer provides custom implementations of these methods. In my opinion, the others, however, should be refactored in such a way that they are not accessible in userland, or they should be documented.

[cjihrig]

I did something similar to this recently with the readline module. The process was to:

1. Identify userland usage for each method.
2. Identify which ones should be publicly exposed and which should not.
3. Document the public API methods.
4. Move the others to an internal module and begin deprecating.

[lance]

@cjihrig how on earth do you identify overall userland usage for each method? Surely there's a tool I can use. Can you point me in the right direction?

[cjihrig]

We have a very scientific method of pinging @ChALkeR and asking him to look it up.

[MylesBorins]

@ChALkeR would you be willing to write a document on your process?

[lance]

@mscdex the reason I added the meta tag is that I am also curious about process and/or documentation in the future about best practices among new contributors wrt code style. This may be addressed somewhere already. My experience has been primarily code style enforced by linting - which is good, but I think insufficient.

Maybe these concerns just come out in the LGTM reviews for new pull requests, and that's all of the ceremony needed around it. I just wonder if some semantic code guidelines would be useful.

[lance]

@ChALkeR ping - I'll be happy to take on the work if there's a reasonable way to do this. Noodled a few ideas with some coworkers yesterday, but nothing that was simple and straightforward jumped out.

[lance]

@ChALkeR just following up. Any chance you can lend a hand here?

[ChALkeR]

Ah, sorry, I missed this. Will be ready today =).

[ChALkeR]

Note: everything here is based on the 2016-01-28 dataset. I collected the newer one (at 2016-07-23), but didn't have enough time to upload it before traveling to another city, so it was just left there on my PC in the last moment :-). I will either rebuild a newer dataset directly on the server or will just use the old one for a few more weeks.

First of all, instances of repl.start (actually, ([rR][eE][pP][lL]|require.*repl.*)\.start) calls are here, to understand what we are working with. Modules extracted from that list — here. All modules that require repl (or anything else similarly named) — here. Their union — here.

• setPrompt and close are hard to check, as readline also has those.
  setPrompt mentions are here. Interersection with the modules list above gives us this list. It's definitely non-empty.

  Note that REPLServer is documented to inherit from readline.Interface, and those are documented methods of readline.Interface. I'm pretty sure those should be viewed as being documented.

• convertToContext doesn't look used, see also repl: deprecate unused function convertToContext #7829 (comment). The PR to hard deprecate and removal in the future LGTM. Grep here.

• parseREPLKeyword — here. Only nish seems to use in a way that doesn't copy the repl source code. Also looks good to me to hard deprecate and remove it in the future.

• .memory( calls on anything — here. but only a small part of that is on repl. Intersection (quite small) here, and most of those are node repl copies again. Not all, though.

  Still, not sure what to do with that method. Is it worth being documented? I.e. does it bring some functionality that we would want to expose and document?

• .createContext( is hard to search — it's a popular name, e.g. used in vm and other places. Searching for \.createContext[^a-zA-Z], removing what definitely looks like calls on vm. ((vm_?|Script)\.createContext) and intersecting with the module list gives us this list, which looks like just node repl copies and false positives. Most probably it would be fine to hard deprecate that, if we are sure that it shouldn't be documented and exposed.

• .resetContext — here. Filtered — here. Most of those look like node repl copies, but ejdb doesn't. Most probably it would be fine to hard deprecate that, if we are sure that it shouldn't be documented and exposed.

[ChALkeR]

@lance @thealphanerd @cjihrig
Sorry for the delay again. Wish GitHub had a different category for personal mentions in the notifications.