Skip to content
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

Add JavaDoc #39

Merged
merged 13 commits into from
Aug 12, 2022
Merged

Add JavaDoc #39

merged 13 commits into from
Aug 12, 2022

Conversation

Johannes-Schneider
Copy link
Contributor

@Johannes-Schneider Johannes-Schneider commented Jul 29, 2022

This PR adds JavaDoc to all public APIs.

Release Notes

Module: java-access-api

  • Breaking Changes:
    • Removed the default constructor of com.sap.cloud.environment.servicebinding.api.exception.ServiceBindingAccessException.

Module: java-consumption-api

  • TypedMapView#getEntries now also returns all entries that are subtypes (Class#isAssignableFrom) of the queried entry type.
  • TypedListView#getItems now also returns all items that are subtypes (Class#isAssignableFrom) of the queried item type.
  • Breaking Changes:
    • Removed the default constructor of com.sap.cloud.environment.servicebinding.api.exception.ValueCastException.
    • Removed the default constructor of com.sap.cloud.environment.servicebinding.api.exception.KeyNotFoundException.

Module: java-sap-service-operator

  • Breaking Changes:
    • Following classes have been removed from the public API:
      • com.sap.cloud.environment.servicebinding.metadata.BindingMetadata
      • com.sap.cloud.environment.servicebinding.metadata.BindingMetadataFactory
      • com.sap.cloud.environment.servicebinding.metadata.BindingProperty
      • com.sap.cloud.environment.servicebinding.metadata.PropertyFormat

@Johannes-Schneider Johannes-Schneider marked this pull request as ready for review July 29, 2022 15:05
Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

braindump

* inside this class can both be retrieved ({@link DefaultServiceBindingAccessor#getInstance()}) <bold>and</bold>
* manipulated ({@link DefaultServiceBindingAccessor#setInstance(ServiceBindingAccessor)}). Applications might want to
* overwrite the default instance during startup to tweak the default behavior of libraries that are relying on this
* fallback. <br>
Copy link
Member

Choose a reason for hiding this comment

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

Where is the term "fallback" coming from? I only see that you can change the statically stored instance...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to improve the wording.

* fallback. <br>
* <bold>Please note:</bold> It is considered best practice to offer APIs that accept a dedicated
* {@link ServiceBindingAccessor} instance instead of using the globally available instance stored inside this class.
* For example, libraries that are using {@link ServiceBindingAccessor}s should offer APIs such as the following:
Copy link
Member

Choose a reason for hiding this comment

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

I think that if you offer to manage the global state there is a good chance people will use that, regardless of what the javadoc states 😉

Anyhow, for that purpose the naming "accessor" sounds somewhat odd..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyhow, for that purpose the naming "accessor" sounds somewhat odd..

I've created an issue to consider renaming the Accessors.

import com.sap.cloud.environment.servicebinding.api.ServiceBinding;

/**
* A {@link LayeredParsingStrategy} that expects all property files to contain "plain" text (i.e. no JSON structures).
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add an @see to the interface. The second sentence doesn't really make sense to me, I mean all properties are recognised by their name. In what format should credentials and metadata be? are they directories instead?

Copy link
Contributor Author

@Johannes-Schneider Johannes-Schneider Aug 8, 2022

Choose a reason for hiding this comment

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

Maybe add an @see to the interface.

Where would you point to?

I mean all properties are recognised by their name.

Not really. The LayeredSecretKeyParsingStrategy, for example, distinguishes metadata and credentials properties by their structure instead - the one and only JSON file contains the credentials whereas all other (plain text) files contain metadata. The name of the credentials file doesn't matter.

In what format should credentials and metadata be? are they directories instead?

Not sure what you mean here... The doc already says "property files [...] contain "plain" text (i.e. no JSON structures).".
I thought that would be pretty clear?

Copy link
Member

Choose a reason for hiding this comment

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

yes the contrast to the other strategy is helpful here 👍🏻

Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

I see you also ran the formatter, nice! LGTM, I think the Javadoc is adding a lot of clarity!

import com.sap.cloud.environment.servicebinding.api.ServiceBinding;

/**
* A {@link LayeredParsingStrategy} that expects all property files to contain "plain" text (i.e. no JSON structures).
Copy link
Member

Choose a reason for hiding this comment

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

yes the contrast to the other strategy is helpful here 👍🏻

@Johannes-Schneider Johannes-Schneider merged commit 18a919d into main Aug 12, 2022
@Johannes-Schneider Johannes-Schneider deleted the add-javadoc branch August 12, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants