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

catalog: Cleanup catalog api -- remove private apis and export missin… #66

Merged
merged 5 commits into from Sep 3, 2019

Conversation

@sbrossie
Copy link
Member

commented Aug 27, 2019

…g public apis. See killbill/killbill#1189

@pierre
pierre approved these changes Aug 27, 2019
@@ -23,6 +23,8 @@
*/
public interface PlanPhase extends CatalogEntity {

public StaticCatalog getCatalog();

This comment has been minimized.

Copy link
@pierre

pierre Aug 27, 2019

Member

Should this be in CatalogEntity instead?

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

I started that way as well but CatalogEntity i used by many other catalog resource that currently don't maintain a pointer to their static catalog.

@@ -70,6 +72,7 @@
*/
public Plan createOrFindCurrentPlan(PlanSpecifier spec, PlanPhasePriceOverridesWithCallContext overrides) throws CatalogApiException;

// TODO_CATALOG All these exceptions are a result of having our DefaultVersionedCatalog implementation implement both StaticCatalog and Catalog (and are needed for the Catalog case)

This comment has been minimized.

Copy link
@pierre

pierre Aug 27, 2019

Member

Maybe keep these comments outside of the API files (to avoid confusing the user), since they are implementation details.

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

yep, and issue is gone anyways.

sbrossie added 2 commits Aug 30, 2019
@pierre
pierre approved these changes Sep 2, 2019
Copy link
Member

left a comment

Very nice! 👌

* @return the catalog version matching this date
*
*
* Assuming two StaticCatalog, S1 with a version date of D1, and S2 with a version date of D2:

This comment has been minimized.

Copy link
@pierre

pierre Sep 2, 2019

Member

Nit: not 100% sure (build warnings would probably tell you), but I think the Javadocs description should be above the @param list.

*
* Assuming two StaticCatalog, S1 with a version date of D1, and S2 with a version date of D2:
* - Specifying a date D such that D1 <= D < D2 will return S1
* - Specifying a date D such that D2 <= D will return S2

This comment has been minimized.

Copy link
@pierre

pierre Sep 2, 2019

Member

Just for completeness, add an example for D < D1.

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

Ack -- added.

import java.util.Date;
import java.util.List;

public interface VersionedCatalog {

This comment has been minimized.

Copy link
@pierre

pierre Sep 2, 2019

Member

Nit: should we add a getName() API (since it's the same name across all versions)?

This comment has been minimized.

Copy link
@sbrossie

sbrossie Sep 3, 2019

Author Member

Ack will add.

@sbrossie sbrossie merged commit 91f39a0 into work-for-release-0.21.x Sep 3, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.