Resolve link warnings #1994
Resolve link warnings #1994
Conversation
45930
requested review from
hattyhattington17 and
querolita
and removed request for
a team
| @@ -231,7 +231,7 @@ let sideloadedKeysCounter = 0; | |||
| * NOTE: In the case of `DynamicProof`s, the circuit makes no assertions about the verificationKey used on its own. | |||
| * This is the responsibility of the application developer and should always implement appropriate checks. | |||
| * This pattern differs a lot from the usage of normal `Proof`, where the verification key is baked into the compiled circuit. | |||
| * @see {@link src/examples/zkprogram/dynamic-keys-merkletree.ts} for an example of how this can be done using merkle trees | |||
| * @see `src/examples/zkprogram/dynamic-keys-merkletree.ts` for an example of how this can be done using merkle trees | |||
There was a problem hiding this comment.
removing this link, which was not functional in any case.
| @@ -18,7 +18,7 @@ import { Bytes } from '../bytes.js'; | |||
| export { createForeignCurve, ForeignCurve }; | |||
|
|
|||
| // internal API | |||
| export { toPoint, FlexiblePoint }; | |||
| export { toPoint, FlexiblePoint, ForeignCurveNotNeeded }; | |||
There was a problem hiding this comment.
By exporting ForeignCurveNotNeeded, we make it linkable in other comments, where it is already in use. This makes the whole chain public.
ForeignCurveNotNeeded in particular seems weird. I'm not sure if we should just delete it in favor of ForeignCurve. See: this PR which introduces it.
| @@ -421,10 +425,10 @@ class ForeignCurveNotNeeded extends ForeignCurve { | |||
| * const Curve = createForeignCurve(Crypto.CurveParams.Secp256k1); | |||
| * ``` | |||
| * | |||
| * `createForeignCurve(params)` takes curve parameters {@link CurveParams} as input. | |||
| * `createForeignCurve(params)` takes curve parameters `CurveParams` as input. | |||
There was a problem hiding this comment.
Removing the link to CurveParams here so that createForeignCurve can remain public, and we don't also have to make CurveParams public. This is my opinionated way of resolving, since the type CurveParams is linked in the function signature, it is quite a similar experience for the developer.
| * - When constructing from another {@link ForeignField} instance, ensure the modulus matches. If not, check the modulus using `Gadgets.ForeignField.assertLessThan()` and handle appropriately. | ||
| * - When constructing from a {@link Field3} array, ensure all elements are valid Field elements and range checked. | ||
| * - When constructing from a `Field3` array, ensure all elements are valid Field elements and range checked. |
There was a problem hiding this comment.
Removing the link to Field3 here. The type is used in the function signature. The outcome here is that new ForeignField() remains public, and Field3 is private.
| @@ -19,7 +19,10 @@ | |||
| }, | |||
| "entryPoints": [ | |||
| "src/index.ts", | |||
| "src/lib/mina/account-update.ts", | |||
| "src/lib/provable/crypto/foreign-curve.ts", | |||
There was a problem hiding this comment.
Foreign curves are already public: https://docs.minaprotocol.com/zkapps/o1js-reference/classes/ForeignCurve
This change makes all the doc comments exposed in the file available for other files to reference in @links.
45930
requested review from
ymekuria and
boray
and removed request for
hattyhattington17 and
querolita
| @@ -77,7 +77,7 @@ class UInt64 extends CircuitValue { | |||
| return this.value.toString(); | |||
| } | |||
| /** | |||
| * Turns the {@link UInt64} into a {@link BigInt}. | |||
| * Turns the {@link UInt64} into a BigInt. | |||
There was a problem hiding this comment.
I'm curious why you removed the BigInt link?
There was a problem hiding this comment.
BigInt is also invisible to the docs, so a typedoc warning is raised here.
I thought BigInt is generic enough that people know what we mean, but I could replace this with an http link to some bigint docs, or look for another workaround to keep the intellisense link here.
There was a problem hiding this comment.
It would also be valid to turn this rule off like we did with notExported in #1993. I think it is worth knowing if there is a typo in a link or if a public API is referencing private APIs, and resolving those, rather than ignoring. But if it becomes too much effort, or too much value is lost by working around this rule, we can just turn it off too.
This PR resolves warnings related to
{@link}tags linking to things that are not documented.This problem requires some finesse, as we use link tags for intellisense on symbols that aren't linked in the documentation site. We also use links on symbols that are on the site, but link to things that aren't.
An easy fix for the internal-only use case is to tag the doc comment with
@internal, so the intellisense will work and the site generator will know to skip it.The trickier case is when a public comment references an internal API. For these, we can either make the whole chain public, make the whole chain private, or remove the link tag, and leave the reference to a symbol that is internal unlinked. I will call out these cases in inline comments.
This PR also resolves all warnings from executing
npx typedoc, so we could conceivably add CI to keep it that way.