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

Enum descriptions #13

Open
viceroypenguin opened this issue Apr 28, 2021 · 13 comments
Open

Enum descriptions #13

viceroypenguin opened this issue Apr 28, 2021 · 13 comments

Comments

@viceroypenguin
Copy link

The following enums do not have descriptions:

  • AccountSubtype
  • CountryCode
  • Products
  • BankTransferType
  • BankTransferStatus
  • BankTransferNetwork
  • BankTransferEventType (description for items, but not for enum itself)

While most of these are fairly self evident, it makes it more difficult to add documentation on generated code.

@cgfarmer4
Copy link
Contributor

@phoenixy1 perhaps we could add descriptions to these?

@phoenixy1
Copy link
Contributor

@viceroypenguin OpenAPI doesn't seem to support adding descriptions to enums. From https://swagger.io/docs/specification/data-models/enums/ "If you need to specify descriptions for enum items, you can do this in the description of the parameter or property". Is the request to enumerate all the possible enums in the description field?

@viceroypenguin
Copy link
Author

@phoenixy1 - Only looking for descriptions similar to other enums already present in the .yaml file, such as for ACHClass, among others. My parsing routine can decipher those descriptions to have value descriptions for each of the enum values, but it would be nice to have the same for these enums.

@phoenixy1
Copy link
Contributor

phoenixy1 commented Jun 10, 2021

@viceroypenguin OK, so it sounds like the request is that the descriptions should be formatted like they are for ACHClass, in order to make it easier to parse them? AccountSubtype and BankTransferType for example do have descriptions, but they are formatted differently. To go through these one by one because they are all slightly different:

AccountSubtype: I don't think we can change this one to match that format, it would take up waaaay too much space in the documentation.

BankTransferStatus: It might be worth changing this, the fields are mostly self-explanatory but not totally.

BankTransferType: If the ask is just to reformat the description, I can easily do that.

BankTransferNetwork, CountryCode, Products: Are you already parsing the list of possible enum values and putting that in the docs? (Our API docs currently do that.) If you are, I'm not sure it would add value to do that for some of these enums that are totally (or (99%) self-explanatory and where we have no real descriptive information to add.

BankTransferEventType: Not sure I understand the request here -- the description seems to be in the right place, as far as I can tell?

@viceroypenguin
Copy link
Author

AccountSubtype: The reason for the ask is to (in line with the vision of providing the openapi) eventually automate generating the entities used in the supporting library. Currently I am parsing the documentation directly from the .yaml file to apply to the enum values directly, so that users will have information present in the code as they use it. However, the AccountSubtype exception would mean that for each upgrade I would need to manually go to the plaid docs and provide documentation for new entries (such as recently added life insurance, etc.). Alternatively, I could leave documentation as simple the enum value and a link to the plaid website, but that seems like an extra step. Is there any other place where the AccountSubtype information could be automatically collected and applied.

Actually, given the nature of AccountSubtype having a different documentation nature, maybe that would be best anyway?

BankTransferStatus, BankTransferType: Yes please

BankTransferNetwork, CountryCode, Products: I am currently parsing the description field of the enum definition and extracting documentation of the enum values and applying them to the values directly. Understood there may not be more information to describe these than already present, but it would still be useful to have available. For example, the comment on Products about the "Balance" product would be useful on the Balance enum value, etc. This would be especially important if the descriptions change in the future.

BankTransferEventType: Yes, the enum values are described, but there is no description for the enum type itself. Even something as simple as "The type of event that this bank transfer represents" or similar, same as you do for other enum types.

Let me know if these aren't clear enough or if could explain better?

@phoenixy1
Copy link
Contributor

phoenixy1 commented Jun 10, 2021

AccountSubtypes: These are defined in the members of the StandaloneAccountType schema object, is there anything useful you can do with that? (We created this object as a slight abuse of the OpenAPI format in order to make pretty tables in the API docs to show account types and subtypes.)

BankTransferStatus, BankTransferType: Will do.

BankTransferNetwork, CountryCode, Products: Will take this back to the team to think about...for stuff like the Products and CountryCode enum it's basically a tradeoff between making the information easier to parse in an automated way and taking up extra space on the API Reference docs page. BankTransferNetwork is no big deal because there are only three but for products there are like 12 so adding descriptions for all of them makes me wince a little.

BankTransferEventType: Oh, that's all you needed? Got it. Commit in progress!

@viceroypenguin
Copy link
Author

StandaloneAccountType - I'll definitely take a look at this. It'll probably get me what I need. I just have to figure out how to special-case this one. Thanks!

@viceroypenguin
Copy link
Author

@phoenixy1 Further question - there are other enums throughout the system that are not created as separate enums like AccountType, Products, etc. Several examples of these in the transaction object: PaymentChannel, TransactionCode, etc. There are more in other objects.

  1. is there any chance these could be converted to enum-type objects like the ones already created?
  2. if so, then would it be helpful for me to identify these for you guys?

If not, I'll look at other ways to parse these into separate enums on my side.

Thanks!

@viceroypenguin
Copy link
Author

@phoenixy1 PS: Thanks for the updates you've done already for BTS, BTT, etc.

@phoenixy1
Copy link
Contributor

@viceroypenguin hmm...is it important for these to be separate enums? generally we've been separating stuff out into separate objects it's reused somewhere but doing them as one-offs if it's not.

we can do this if there aren't too many of them, it would be helpful for you to identify them.

@viceroypenguin
Copy link
Author

I'll review and get back to you. I figured out how to render them, so it may just be a matter of titling them in some cases for cleaner type-naming.

@phoenixy1
Copy link
Contributor

@viceroypenguin we are considering removing enums from the openapi file altogether because we consider enums extensible but it turns out that the openapi spec does not support extensible enums. We fixed it for our own libraries by removing validation on enums but are worried it might bite community developers like you. Do you have any opinion on this one way or another?

@viceroypenguin
Copy link
Author

@phoenixy1 Yes, removing enums would make life difficult for C# developers at least. Take the /link/token/create api as an example. There are several fields that require a specific subset of string values: language, country_codes, and products to start.

With enums, I can specify as the library developer what the allowed values are, and the consumer coder can know the range of potential values and write code to provide those values quickly and efficiently. It also reduces the amount of trial and error from users forgetting the value, mis-typing the value, or mis-understanding the value.

Without enums, these values would be string in all cases, and developers would be forced to have a copy of the api open to look-up the allowed range of values for each field. If entered incorrectly, requires additional round-trips of debugging work to identify the error and correct.

There are two possibilities on our side to deal with it, neither of which is as clean as enums: we could specify a constants table and encourage (but not require) consumers to use the table; or we could include the range of values in the comments of each field. Either of these options could reduce the number of errors, but neither is as effective at early detection of invalid values as enums are.

Having said all of the above, I'll counter-argue: if the enum properties are supposed to be extensible (i.e. you may add values on the fly), then yes, enums would create validation issues if a new value is provided that is not in the list of expected potential values.

Maybe a compromise? Any properties that consumers are expected to provide could be enums, but other properties just have a non-exclusive description of potential values.

This would allow enforcement of proper set of values for input to the Plaid api, but allow flexibility in the output of the Plaid api. This would not be restrictive, because if a new value is allowed for input, then the next library update can include the new values, but no existing code will be hurt; but if new values are allowed for output, the consuming code will know to have an exception case for unexpected values and deal with them appropriately (whether pass-thru for display or ignore the response or whatever makes sense for the particular property).

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

No branches or pull requests

3 participants