Skip to content

Match sub-classing interfaces #338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
npepinpe opened this issue Jan 30, 2025 · 4 comments
Open

Match sub-classing interfaces #338

npepinpe opened this issue Jan 30, 2025 · 4 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Help on this is appreciated

Comments

@npepinpe
Copy link

Description

We use MeterDocumentation with enums to document our metrics, and generally quite like the pattern, as it provides some technical documentation for all our metrics. We extend MeterDocumentation however to add more information. While it would be nice to have that shown by the generator, this is not the problem.

The main issue is that the generator doesn't pick up enums implementing the extending interface, it only picks up enums which directly implement MeterDocumentation.

Take the following ExtendedMeterDocumentation interface:

/**
 * Extends the base {@link MeterDocumentation} API to allow for more static description, e.g.
 * help/description associated with a given metric.
 */
public interface ExtendedMeterDocumentation extends MeterDocumentation {
   final Duration[] EMPTY_SLOS = new Duration[0];

  /** Returns the description (also known as {@code help} in some systems) for the given meter. */
  String getDescription();

  /**
   * Returns the buckets to be used if the meter type is a {@link Meter.Type#TIMER} or {@link
   * Meter.Type#DISTRIBUTION_SUMMARY}.
   */
  default Duration[] getServiceLevelObjectives() {
    return EMPTY_SLOS;
  }
}

And the following enum:

public enum MyMetrics implements ExtendedMeterDocumentation {
  // ... some metrics
}

Naively, I would expect the generator to pick up MyMetrics, but it does not.

As a workaround, I can do the following:

public enum MyMetrics implements MeterDocumentation, ExtendedMeterDocumentation {
  // ... some metrics
}

Then the generator will pick it up. It would be great though if it wasn't necessary to again declare the implementation of MeterDocumentation.

I'm happy to pick this up if you think this would be valuable. I understand there's not much urgency if there is a workaround :)

@shakuzen
Copy link
Member

I'm happy to pick this up if you think this would be valuable.

Thank you for the offer. It sounds worthwhile to me. Please send a PR and we'll review.

@marcingrzejszczak marcingrzejszczak added enhancement New feature or request help wanted Help on this is appreciated good first issue Good for newcomers labels Feb 28, 2025
@npepinpe
Copy link
Author

npepinpe commented Mar 9, 2025

I wasn't familiar before with how the generator worked, but I had a look now. It looks like it parses the files directly checking if specific interfaces are implemented, without actually building the class graph. This means we have no real knowledge during generation outside of what's specified in the source file of the class.

So I see three options:

  1. Add a new option to the generator to allow specifying additional supported interfaces. Optionally we can enforce that these additional interfaces must extend MeterDocumentation, or ObservationDocumentation, etc.
  2. Auto-detect extensions of the supported interfaces by doing a first pass over the source code, then adding any found to the list of supported interfaces.
  3. We can assume that the extension interface is part of the inclusion pattern. This means the type will be picked up by the JavaSourceSearchHelper, so we can auto detect these when building the search helper initially, and look them up via new method (e.g. #searchSupportedInterfaces or something).

The first one is likely simpler, but less user friendly. The second one is probably slower, more complex, but a better UX. Third one is just a less flexible variant of the second one, and is probably simpler to implement, but less flexible in terms of UX.

Thoughts? Maybe you also have a different idea of how this could be implemented.

@shakuzen
Copy link
Member

I'm distracted with preparing some things for upcoming releases, so I can't dig into this right now to give an informed opinion. Pinging @ttddyy in case he has any thoughts.

@ttddyy
Copy link
Contributor

ttddyy commented Mar 10, 2025

I think auto-detecting (choice 2) wouldn't be a bad approach since it's only targeting enums. The number of interfaces implemented by enums should be relatively small, so traversing them wouldn't be too heavy of an operation.

For all three choices, another consideration is retrieving values from methods like getName or getContextualName. This requires traversing the hierarchy and selecting the closest overridden method to the enum. Essentially, it needs to mimic the compiler's behavior, similar to what a search helper does and you should be able to leverage existing methods in the search helper for this. Caching this information during the first traversal and encapsulating necessary details into a single class would likely be beneficial - similar to how MergedContextConfiguration in the Spring Test Context Framework synthesizes required attributes into a single value class. Any method-checking or retrieval logic should be updated accordingly to account for this hierarchy traversal.

Overall, I think auto-detection and hierarchical value retrieval would give the necessary feature here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Help on this is appreciated
Projects
None yet
Development

No branches or pull requests

4 participants