Skip to content

Conversation

@unchuckable
Copy link
Contributor

Sorry this took a little. Corona and RL made me have to deal with other issues before this one.
Feedback welcome. I tried to provide the functionality of AVRO-1329 along with some suitable yet compatible changes to the public API, but am well willing to make changes as deemed appropriate.

Thanks, and stay safe.

---8<------

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests:
  • TestSchema.testExtendedEnumSchemaSerializeAndDeserialize
  • TestSchema.testSimpleEnumSchemaSerialize
  • Updates/Extensions to Compiler Unit tests

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@github-actions github-actions bot added Java Pull Requests for Java binding website labels Jan 22, 2021
@unchuckable
Copy link
Contributor Author

Dunno what these CI issues are, can anyone help there?

@Fokko Fokko changed the title Avro 1329 extended enums AVRO-1329: Extended enums Jan 29, 2021
@RyanSkraba RyanSkraba self-requested a review February 5, 2021 16:07
@Humbedooh Humbedooh deleted the branch apache:master October 5, 2023 22:04
@Humbedooh Humbedooh closed this Oct 5, 2023
@unchuckable
Copy link
Contributor Author

unchuckable commented Oct 6, 2023

@Humbedooh - Hi, Daniel. Can you please elaborate as to why this ticket is being closed?

@KalleOlaviNiemitalo
Copy link
Contributor

@unchuckable, I guess it's because this pull request targeted the "master" branch, which has now been renamed to "main" (AVRO-3878, #2537). Are you able to retarget this to "main"?

I would like to have "symbol-props" or similar at least added to the Avro specification, even if the per-language libraries were not changed to recognize it. I have a custom application that generates HTML documentation from Avro schemas, and I'd be able to implement "symbol-props" there and then migrate the per-symbol documentation from the "doc" properties of my enum schemas.

@martin-g
Copy link
Member

martin-g commented Oct 6, 2023

@unchuckable
Copy link
Contributor Author

@KalleOlaviNiemitalo, @martin-g Thanks for the explanation. Makes sense. Nice to see not only us have a use case for this. Basically, what I would like to add at a later point, would be enum symbol aliases similar to field aliases, that would help us a lot in schema evolution.
I think I'll try to rebase this to current versions and re-submit a pull request with the new main branch.

@martin-g
Copy link
Member

martin-g commented Oct 6, 2023

Hovering over the Reopen and comment button below shows this hint: The repository that submitted this pull request has been deleted :-/

@unchuckable
Copy link
Contributor Author

Interesting. Something must have been screwed up there. Will recreate the change to the best of my ability ;)

@unchuckable
Copy link
Contributor Author

Found the original code again, will rebase onto current main and resubmit the pull request these days.

@KalleOlaviNiemitalo
Copy link
Contributor

I tried implementing "symbol-props" in my doc generator application. It is a bit easier with the map style

{
    "type": "enum",
    "symbols": ["zero", "one", "two"],
    "symbol-props": {
        "zero": {},
        "one": { "doc": "uno" }
    }
}

than with the array style

{
    "type": "enum",
    "symbols": ["zero", "one", "two"],
    "symbol-props": [
        { "name": "zero" },
        { "name": "one", "doc": "uno" }
    ]
}

because my application can loop over the value of "symbols" and use a JSON DOM method to easily look up the corresponding entry from the "symbol-props" map. In the array style, the symbol names are in the "name" properties, and my application has to construct a dictionary from those before it starts looping over "symbols".

But perhaps consistency with record schemas is more important than convenience here.

@KalleOlaviNiemitalo
Copy link
Contributor

Found the original code again, will rebase onto current main and resubmit the pull request these days.

If you had not found the original code, you could still have fetched it from GitHub, e.g. by running git fetch https://github.com/apache/avro/ refs/pull/1067/head. That ref is listed by git ls-remote.

@KalleOlaviNiemitalo
Copy link
Contributor

Please rename "symbol-props" to "symbolProps", for consistency with the "logicalType" attribute. In the current Avro specification, the kebab-case style is used only for names of logical types, e.g. "time-millis".

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 website

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants