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

fix(products): Improve API usability. #572

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kpfleming
Copy link
Contributor

@kpfleming kpfleming commented Dec 17, 2024

  • Add 'EnableOutput' type aliases in each product, so that users of the API don't need to know whether that product has its own EnableOutput or not. Add 'NewEnableOutput' function so that users of the API can easily construct mock output instances.

  • Add 'ProductName' constant in each product, so that users of the API will have access to the canonical name of the product along with its product_id.

  • Change package names to conform to Go guidelines (remove underscores).

  • Change APIs to accept and return by-value instead of by-pointer, as using by-pointer structures can be difficult in generic parameter contexts and the structures involved are small (so there is no real benefit to avoiding by-value passing).

Copy link
Contributor

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

I really like the package restructuring and the consts for product names and IDs. This clarifies things and makes it easier to use this correctly.

I'm not (presently, maybe I'll change my mind?) a fan of the type aliases. It makes it seem like there are a bunch of different/new types when they're actually almost all the same productcore.EnableOutput.

@kpfleming
Copy link
Contributor Author

The motivation for that is when a consumer of bot_management.Enable needs to know the type of the output (they won't always, but for testing they likely do), they would need to know whether that type is products.EnableOutput or bot_management.EnableOutput, and if that changed in the future it would be a breaking change. Aliasing to bot_management.EnableOutput addresses that problem well.

@kpfleming
Copy link
Contributor Author

The ProductOutput and related types have moved back to the fastly/products package, as they are going to be useful in the CLI as a consumer of these APIs.

@kpfleming kpfleming force-pushed the cdtool-966-followup branch from 87532e9 to f29cf77 Compare January 14, 2025 13:55
* Add 'EnableOutput' type aliases in each product, so that users of
  the API don't need to know whether that product has its own
  EnableOutput or not. Add 'NewEnableOutput' function so that users of
  the API can easily construct mock output instances.

* Add 'ProductName' constant in each product, so that users of the API
  will have access to the canonical name of the product along with its
  product_id.

* Change package names to conform to Go guidelines (remove underscores).

* Change APIs to accept and return by-value instead of by-pointer, as
  using by-pointer structures can be difficult in generic parameter
  contexts and the structures involved are small (so there is no real
  benefit to avoiding by-value passing).
@kpfleming kpfleming force-pushed the cdtool-966-followup branch from f29cf77 to 7d432c5 Compare January 14, 2025 19:58
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