Convert vars marked as :no-doc in api.clj to :private where possible

[dgr]

Problem/Opportunity
There are a number of vars in api.clj that are marked as :no-doc. These do not appear in the API docs, but they are still accessible to external code. Where possible, these vars should be converted to :private (or use defn- if functions).
Proposed Solution
Do an analysis of which vars can be converted and convert :no-doc to :private / defn- where possible.

Alternative Solutions
None

Additional context
This is part of a general activity of trying to more accurately define Etaoin's public API.

Action
I'll do an analysis, comment here, and then submit a patch once there is rough agreement on which vars to make :private.

[lread]

Nice to have a fresh set of eyes, I'm so familiar with :no-doc that I forget that some folks might not be; I expect this should be helpful to others.

When they cannot be made private, feel free to move these vars to existing or new impl nses @dgr.

[dgr]

Here are the results of a quick analysis of vars marked as :no-doc.

A few general findings:

1. I specifically looked at vars in the etaoin.api namespace. Only one other namespace has a var marked as :no-doc (etaoin.ide.flow).
2. There are several whole namespaces that are marked as :no-doc, which I assume was done to every namespace that was judged to be implementation rather than public API. I have ignored those namespaces entirely, though sometimes, as you will see, one of those namespaces makes use of a var in the public etaoin.api namespace.
3. The CHANGELOG documents many of the vars listed below as being private and not part of the public API.
4. I looked for usage of :no-doc vars within the Etaoin codebase to help understand just how private a var was. An etaoin.api var marked with :no-doc and only used within the etaoin.api namespace itself is very private. An etaoin.api var marked with :no-doc and used within another Etaoin namespace is private, but less so. I did not search Github or make any other attempt to determine whether a :no-doc var is used "out in the wild."

After this short analysis, I categorized the etaoin.api :no-doc vars into buckets:

Bucket 1: Very private vars

These are vars that are marked as :no-doc and not used in other Etaoin namespaces.

• default-locator: CHANGELOG says this is internal only.
• process-log: CHANGELOG says this is internal only.
• with-exception: CHANGELOG says this is internal only.
• with-locator: CHANGELOG says this is internal only.
• make-screenshot-file-path: CHANGELOG says this is internal only.
• postmortem-handler: CHANGELOG says this is internal only.
• with-headless-driver: CHANGELOG doesn't mention this var one way or another. It's used internally to etaoin.api but not used in other Etaoin namespaces.

I recommend that all the vars in the bucket be marked :private, either through direct metadata or via defn- for functions.

Bucket 2: Semi-private vars

These are vars that are marked as :no-doc in etaoin.api but are used in other Etaoin namespaces.

• locator-xpath: This has an identical definition in the etaoin.query namespace and is used in that namespace. This var is not marked as :no-doc or :private in etaoin.query. CHANGELOG says that this is internal, however. Under the DRY principle, this should only be defined in one place and used in both etaoin.api and etaoin.query. Given that etaoin.api requires etaoin.query, we can probably delete the var from etaoin.api and use the (identical) definition in etaoin.query. That said, we still have to determine whether this is a public API or not. CHANGELOG says it's not public, but the var is public and visible in etaoin.query.
• locator-css: This is the same situation as with locator-xpath with the same recommendation from me.
• dispatch-driver: This is the dispatch function for a bunch of driver-specific multi-methods. This is defined identically in both etaoin.api, where it is marked as :no-doc, and in etaoin.impl.driver, where is it public. CHANGELOG says that this is a private function. Under the DRY principle, we should probably just define this in one place. Since etaoin.api requires etaoin.impl.driver, I recommend we remove the definition of dispatch-driver from etaoin.api and reference the definition in etaoin.impl.driver where needed in etaoin.api.
• find-elements*: This is the standard internal function that maps to the WebDriver Find Elements endpoint. This is listed in CHANGELOG as internal-only, but is referenced once in etaoin.ide.impl.api (essentially etaoin.ide.impl.api using a backdoor private API in etaoin.api). It may be possible to eliminate this reference. If somebody has ideas how to best do this, please describe them here. I'm not very familiar with anything in the etaoin.ide.* namespaces.
• switch-frame*: As with find-elements*, this is referenced once in etaoin.ide.impl.api. It is not listed as private in CHANGELOG, however. That said, it carries a "*" suffix in the name, which suggests that it was meant to be private. As with find-elements*, it might be possible to rewrite the usage in etaoin.ide.impl.api to eliminate it.

Conclusion

Make everything in Bucket 1 private. Things in bucket 2 require some larger restructuring of the Etaoin code base to move duplicate definitions into common implementation-only namespaces under etaoin.impl.*. I'm thinking this is a situation where the eventual fix happens over multiple PRs.

[lread]

Thanks for the thorough analysis, Dave.

Anything marked with :no-doc is an implementation detail, not part of the supported Etaoin public API, and therefore OK for us to change freely. I.e. if folks are using something internal, we don't care if we break their usage.

Your effort here is to make it clearer what is internal for folks not familiar with :no-doc, and this is fine, and a nice cleanup, but an alternative would be to educate folks on what :no-doc means.

I'm not at all against you proceeding, but thought I'd raise the alternative.

Make everything in Bucket 1 private.

Sounds good to me.

For bucket 2...

• locator-xpath: This has an identical definition in the etaoin.query namespace and is used in that namespace. This var is not marked as :no-doc or :private in etaoin.query. CHANGELOG says that this is internal, however. Under the DRY principle, this should only be defined in one place and used in both etaoin.api and etaoin.query. Given that etaoin.api requires etaoin.query, we can probably delete the var from etaoin.api and use the (identical) definition in etaoin.query. That said, we still have to determine whether this is a public API or not. CHANGELOG says it's not public, but the var is public and visible in etaoin.query.
• locator-css: This is the same situation as with locator-xpath with the same recommendation from me.

We had decided in the past to expose etaoin.query, we did not clearly understand why, but some folks were using it. We can leave it as part of our public API to avoid breaking them.
Feel free to DRY it up.

• dispatch-driver: This is the dispatch function for a bunch of driver-specific multi-methods. This is defined identically in both etaoin.api, where it is marked as :no-doc, and in etaoin.impl.driver, where is it public. CHANGELOG says that this is a private function. Under the DRY principle, we should probably just define this in one place. Since etaoin.api requires etaoin.impl.driver, I recommend we remove the definition of dispatch-driver from etaoin.api and reference the definition in etaoin.impl.driver where needed in etaoin.api.

Everything in impl nses is not part of the Etaoin public API regardless of wether or not it reaches into other nses. Feel free to DRY it up if that works nicely.

• find-elements*: This is the standard internal function that maps to the WebDriver Find Elements endpoint. This is listed in CHANGELOG as internal-only, but is referenced once in etaoin.ide.impl.api (essentially etaoin.ide.impl.api using a backdoor private API in etaoin.api). It may be possible to eliminate this reference. If somebody has ideas how to best do this, please describe them here. I'm not very familiar with anything in the etaoin.ide.* namespaces.
• switch-frame*: As with find-elements*, this is referenced once in etaoin.ide.impl.api. It is not listed as private in CHANGELOG, however. That said, it carries a "*" suffix in the name, which suggests that it was meant to be private. As with find-elements*, it might be possible to rewrite the usage in etaoin.ide.impl.api to eliminate it.

These might not be ideally placed, but I don't see these as "backdoor". They are just not part of the public API. They are implementation details that are used by another ns.
You could move these to a new etaoin.impl.api if that works cleanly.

[dgr]

Looks like process-log is actually in Bucket 2, not Bucket 1. It's used by the etaoin.dev namespace.