Skip to content

Conversation

@opwvhk
Copy link
Contributor

@opwvhk opwvhk commented May 30, 2025

What is the purpose of the change

Implement AVRO-4147 by updating the javadoc for the SchemaFormatter interface

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@github-actions github-actions bot added the Java Pull Requests for Java binding label May 30, 2025
Copy link
Contributor

@nandorKollar nandorKollar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though it appears to me, that SchemaFormatterFactory is located via ServiceLoader, not the SchemaFormatter implementations. Nevertheless, eventually the factory is used to create the formatters, so it might worths mentioning service loading here too.

@opwvhk
Copy link
Contributor Author

opwvhk commented May 30, 2025

Thank you @nandorKollar !
Your comment is entirely correct, and the javadoc for SchemaFormatterFactory already mentions it. The reason for this comment however, is because I encountered a class loading issue when trying to use the SchemaFormatter in a non-standard environment (an IDE plugin).

The reason it didn't work, is because the class needed to be loaded using the context ClassLoader: neither my plugin nor Avro were available at application start, so the application ClassLoader could not find it.

I'm not certain though, whether I should mention that explicitly or if a reference to the ServiceLoader is sufficient.

@nandorKollar
Copy link
Contributor

Thank you @nandorKollar ! Your comment is entirely correct, and the javadoc for SchemaFormatterFactory already mentions it. The reason for this comment however, is because I encountered a class loading issue when trying to use the SchemaFormatter in a non-standard environment (an IDE plugin).

The reason it didn't work, is because the class needed to be loaded using the context ClassLoader: neither my plugin nor Avro were available at application start, so the application ClassLoader could not find it.

I'm not certain though, whether I should mention that explicitly or if a reference to the ServiceLoader is sufficient.

I think it makes sense briefly highlighting this scenario, that could clarify why ServiceLoader is mentioned here too.

@nandorKollar
Copy link
Contributor

@opwvhk one more thing: I think you should run mvn spotless:apply to fix style violations.

@opwvhk
Copy link
Contributor Author

opwvhk commented May 30, 2025

🤦 spotless...

@opwvhk opwvhk merged commit c5ec956 into apache:main May 30, 2025
8 checks passed
@opwvhk opwvhk deleted the AVRO-4147-SchemaFormatter-javadoc-ServiceLoader branch May 30, 2025 15:48
opwvhk added a commit that referenced this pull request Jun 13, 2025
Mention ServiceLoader in SchemaFormatter javadoc, with special mention of which ClassLoader is used to load formats.

(cherry picked from commit c5ec956)
opwvhk added a commit to opwvhk/avro that referenced this pull request Sep 5, 2025
)

Mention ServiceLoader in SchemaFormatter javadoc, with special mention of which ClassLoader is used to load formats.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java Pull Requests for Java binding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants