Start javadocs on Identifier by cassiancc · Pull Request #2 · FabricMC/annotater

cassiancc · 2026-10-27T20:04:03Z

Not comprehensive - opening this PR as a test of the editing/submission process.

Earthcomputer

I'm surprised the javadoc lint was okay with [a-z0-9/._-], I'd have thought that would be interpreted as a link

modmuss50

Sorry for being picky, but I think we need to be on these early PRs, Ill have a PR with some convensions soon so hopefully that will help.

Co-authored-by: Joseph Burton <burtonjae@hotmail.co.uk>

cassiancc · 2026-10-28T16:13:59Z

Picky is good - didn't know about @throws existing.

ErrorCraft · 2026-11-01T16:07:42Z

I recommend looking at this article as it explains the available keywords, as well as this article for markdown formatting as some keywords (like @code and @literal) are no longer necessary as a result of markdown Javadocs. It might also be useful to link the articles in the conventions PR modmuss mentioned.

As for this earlier comment:

I'm surprised the javadoc lint was okay with [a-z0-9/._-], I'd have thought that would be interpreted as a link

It's 'accepted' but renders the text red as it can't find the reference, as [whatever] tries to reference whatever the same way {@link whatever} does, so [Object#hashCode()] refers to the hashCode method in the Object class.

Earthcomputer · 2026-11-01T16:20:26Z

The javadoc lint is supposed to check that references are valid...

ErrorCraft · 2026-11-01T16:34:36Z

Ah. If you're talking about the changes from #4, this branch doesn't have those commits, so it's not checking anything.

Earthcomputer · 2026-11-01T16:35:03Z

No, the javadoc lint was added in #1

ErrorCraft · 2026-11-01T17:33:01Z

The task only seems to fail for cases where the body has a correct reference format, but the reference itself is invalid. I think it passes because a-z0-9/._- isn't an actual reference to anything, so it (incorrectly?) falls back to the usual markdown link title, or even interprets it as a literal as that is also considered valid markdown. (Example: 'hello [square] world' vs hello [square] world))

A few tests with these references gave the following results:

[Object#hashCode()] - does not fail as expected
[text label][Object#hashCode()] - does not fail as expected
[invalid] - fails as expected (wants to reference invalid as either class, field or method)
[invalid.] - does not fail
[text label][invalid] - fails as expected
[text label][invalid.] - does not fail

Earthcomputer · 2026-11-02T01:15:52Z

I have tested it, the javadoc tool only treats it as a link if the text inside the [] brackets is a valid Javadoc link, otherwise it treats it as plain text. The output has no red text and isn't treated as an error.

cassiancc · 2026-11-02T03:31:45Z

Sorry - why would it be read as a link in the first place? It's wrapped in a code block.

Earthcomputer · 2026-11-02T14:19:11Z

We're talking about before it was wrapped in a code block and worrying about why it wasn't caught by any lint. It might be something for @its-miroma's linter to catch

its-miroma · 2026-11-02T14:50:06Z

markdownlint does not know what a valid Java reference is. I would add a custom linter rule that validates each reference link.

ErrorCraft · 2026-11-02T21:48:31Z

It seems to be pretty inconsistent in IntelliJ too...

its-miroma

This review of mine is intentionally way too nitpicky, because we need to properly establish the conventions to follow.
My comments aren't necessarily changes to be applied, some are food for thought and discussion.

its-miroma · 2026-11-07T11:58:13Z

@@ -0,0 +1,40 @@
+CLASS net/minecraft/resources/Identifier
+	COMMENT `Identifier`s are used in Minecraft's code to identify a resource's location. 


The Docs' refreshed guidelines say: "Avoid specifying that the [modded] game is Minecraft"

Should we apply that rule to Annotater too?

IMO, we should not apply that to the annotator, since the code is not modified yet.

That's even more of a reason why mentioning "Minecraft" is redundant.

Annotater's guidelines don't specify this, I think the clarification is fine.

its-miroma · 2026-11-07T12:00:51Z

+	COMMENT To avoid ambiguity, they consist of a namespace representing the creator of the resource and the path to the resource. 
+	COMMENT When converted to a string, the namespace and path are combined becoming `namespace:path`.
+	METHOD assertValidPath (Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;
+		COMMENT @throws IdentifierException If the path contains a non `[a-z0-9/._-]` character.


I'm not super familiar with javadocs conventions, is this supposed to be read as a sentence? Should "If" be lowercase?

This is an internal private method, should it be documented?

Technically all Minecraft methods are "internal". I don't think we should limit ourselves to only documenting what is public, assuming there is actually something worth documenting.

A private method is only intended to be used by that class, so in that sense it's internal. I think we might want to avoid documenting methods unless they are public or access-widened, to keep the scope of the project contained.

access widened methods do not exist in the annotater realm.

I don't see the harm in documenting private methods if it benefits the user. Neither Yarn or Annotater had restrictions about documenting private methods.

its-miroma · 2026-11-07T12:06:57Z

+		COMMENT @throws IdentifierException If the path contains a non `[a-z0-9/._-]` character.
+	METHOD fromNamespaceAndPath (Ljava/lang/String;Ljava/lang/String;)Lnet/minecraft/resources/Identifier;
+		COMMENT Creates an identifier from a namespace and a path.
+		COMMENT @param namespace The namespace, restricted to `[a-z0-9_.-]`.


The "Effective Dart: Documentation" page that our guidelines cite discourage the usage of @param, especially when the description is redundant. Instead, they suggest formatting it as prose, like:

Both [namespace] and [path] are restricted to `[a-z0-9/._-]`.

In fact, given that we have a @throws clause, in this case we might want to do something like:

@throws IdentifierException if either [namespace] or [path] contain an invalid character.

Question: can multiple @throws clauses reference the same error?

@throws IdentifierException if [namespace] contains an invalid non-`[a-z0-9._-]` character. @throws IdentifierException if [path] contains an invalid non-`[a-z0-9/._-]` character.

Annotater's guidelines do not discourage the use of @param, I think it is a benefit to be able to see descriptions on the parameters themselves.

That's interesting, I was not aware of that detail regarding @param.

its-miroma · 2026-11-07T12:12:52Z

+	METHOD toLanguageKey (Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;
+		COMMENT @return A translation key in the format `prefix.namespace.path.suffix`.
+	METHOD toShortLanguageKey ()Ljava/lang/String;
+		COMMENT @return A translation key in the format `namespace.path`. If the namespace is `minecraft`, the translation key is shortened to `path`.


Should we reference other methods? For example:

Similar to [toLanguageKey], but drops the default [namespace] `minecraft`.

Question: can multiple @return clauses be used in the same comment?

@return A translation key in the format `namespace.path`. @return A translation key in the format `path`, if the [namespace] is {@value #DEFAULT_NAMESPACE}.

~~yes, multiple @return clauses can be used in the same comment and will be formatted like multiple @param clauses.~~

I think referencing other methods makes sense. Multiple @return declarations doesn't though, this isn't allowed per the javadoc spec.

I think referencing other methods makes sense. Multiple @return declarations doesn't though, this isn't allowed per the javadoc spec.

It isn't?

its-miroma · 2026-11-07T12:15:21Z

+		COMMENT @param namespace The namespace, restricted to `[a-z0-9_.-]`.
+		COMMENT @param path The path, restricted to `[a-z0-9/._-]`


Maybe this description of namespace and path, and their constraints, should be moved to the class comment? That way all methods would document what they do with invalid parameters (throw, return null, etc).

Annotater's guidelines allow for repeated descriptions like these - being able to see them when looking at a method is better than having to check the class itself.

Would it make sense to reference the ALLOWED_NAMESPACE_CHARACTERS constant here instead?

its-miroma · 2026-11-07T12:18:27Z

+		COMMENT @param path The path, restricted to `[a-z0-9/._-]`
+		COMMENT @return An `Identifier` created from the namespace and path, or `null` if either string contains an invalid character.
+	METHOD withDefaultNamespace (Ljava/lang/String;)Lnet/minecraft/resources/Identifier;
+		COMMENT Creates an `Identifier` in the vanilla `minecraft` namespace.


Should we reference DEFAULT_NAMESPACE?

The @param description is more verbose than the other comments.

I'm not sure - DEFAULT_NAMESPACE is a variable that's inlined at the compilation stage. We'll need to decide what to do about variables like these that need unpicking.

its-miroma · 2026-11-07T12:21:40Z

+		COMMENT @return A new `Identifier` with the same namespace and a different path.
+	METHOD withPath (Ljava/util/function/UnaryOperator;)Lnet/minecraft/resources/Identifier;
+		COMMENT @param modifier A modifier to prepend to the existing path, with the result restricted to `[a-z0-9/._-]`
+		COMMENT @return Creates a new `Identifier` with the same namespace and a different path.


I don't know if this. is allowed in javadocs like these:

@return a new [Identifier] with the same [this.namespace] and the new [path].

I also think that we should prefer references to code blocks, such as [Identifier] instead of `Identifier`, because the former can be validated.

It is valid by prepending it with a #; [#namespace] refers to the namespace field. As for the method parameters, those aren't valid references within the class, so using inline code is fine.

@return a new [Identifier] with the same [#namespace] and the new `path`.

ErrorCraft · 2026-11-07T21:05:20Z

+		COMMENT @param path the path, restricted to `[a-z0-9/._-]`
+		COMMENT @return an `Identifier` created from the namespace and path, or `null` if either string contains an invalid character.
+	METHOD withDefaultNamespace (Ljava/lang/String;)Lnet/minecraft/resources/Identifier;
+		COMMENT Creates an `Identifier` with the namespace set to [DEFAULT_NAMESPACE].


Suggested change

COMMENT Creates an `Identifier` with the namespace set to [DEFAULT_NAMESPACE].

COMMENT Creates an [Identifier] with the [default namespace][#DEFAULT_NAMESPACE].

Renders like this, which reads more like a sentence:

Creates an Identifier with the default namespace.

Create Identifier.mapping

457114a

Earthcomputer requested changes Oct 28, 2026

View reviewed changes

Comment thread mappings/net/minecraft/resources/Identifier.mapping Resolved

modmuss50 reviewed Oct 28, 2026

View reviewed changes

Update mappings/net/minecraft/resources/Identifier.mapping

b849562

Co-authored-by: Joseph Burton <burtonjae@hotmail.co.uk>

Additional mappings based on review comments

aa8394f

Merge branch '26.3' into test-project

a9ecc71

cassiancc requested review from a team as code owners November 6, 2026 16:54

modmuss50 previously approved these changes Nov 7, 2026

View reviewed changes

NotNightSky previously approved these changes Nov 7, 2026

View reviewed changes

its-miroma reviewed Nov 7, 2026

View reviewed changes

Update Identifier.mapping

9919f58

cassiancc dismissed stale reviews from NotNightSky and modmuss50 via 9919f58 November 7, 2026 20:22

modmuss50 requested a review from Earthcomputer November 7, 2026 20:41

Merge branch '26.3' into test-project

2e5f523

ErrorCraft reviewed Nov 7, 2026

View reviewed changes

		@@ -0,0 +1,40 @@
		CLASS net/minecraft/resources/Identifier
		COMMENT `Identifier`s are used in Minecraft's code to identify a resource's location.

		COMMENT @param namespace The namespace, restricted to `[a-z0-9_.-]`.
		COMMENT @param path The path, restricted to `[a-z0-9/._-]`

	COMMENT Creates an `Identifier` with the namespace set to [DEFAULT_NAMESPACE].
	COMMENT Creates an [Identifier] with the [default namespace][#DEFAULT_NAMESPACE].

Conversation

cassiancc commented Oct 27, 2026

Uh oh!

Earthcomputer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

modmuss50 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cassiancc commented Oct 28, 2026

Uh oh!

ErrorCraft commented Nov 1, 2026

Uh oh!

Earthcomputer commented Nov 1, 2026

Uh oh!

ErrorCraft commented Nov 1, 2026

Uh oh!

Earthcomputer commented Nov 1, 2026

Uh oh!

ErrorCraft commented Nov 1, 2026

Uh oh!

Earthcomputer commented Nov 2, 2026

Uh oh!

cassiancc commented Nov 2, 2026

Uh oh!

Earthcomputer commented Nov 2, 2026

Uh oh!

its-miroma commented Nov 2, 2026

Uh oh!

ErrorCraft commented Nov 2, 2026

Uh oh!

its-miroma left a comment

Choose a reason for hiding this comment

Uh oh!

its-miroma Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cassiancc Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

its-miroma Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NotNightSky Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

its-miroma Nov 7, 2026 •

edited

Loading

cassiancc Nov 7, 2026 •

edited

Loading

its-miroma Nov 7, 2026 •

edited

Loading

NotNightSky Nov 7, 2026 •

edited

Loading