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

Generate Operation IDs when expanding $CRUDL definitions #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pvgoran
Copy link
Contributor

@pvgoran pvgoran commented Jan 19, 2019

Operations generated by expanding $CRUDL definitions gain Operation IDs based on entity name (which is the same as payload key name): createEntity, retrieveEntity, updateEntity, deleteEntity, listEntities.

Operations generated by expanding $CRUDL definitions gain Operation IDs
based on entity name (which is the same as payload key name):
createEntity, retrieveEntity, updateEntity, deleteEntity, listEntities.
@Ajaxy
Copy link
Owner

Ajaxy commented Jan 20, 2019

I believe we need more comprehensive approach here. CRUDL helpers are good to start but often you start needing to replace them with regular definitions.

Also, I think we don’t need to consider the payload key name here, the last endpoint path member seems to be more relevant for the operation ID rather than the params format/schema.

We could come up with a similar algorithm that would handle regular definitions too. For example like this:

POST /users => createUser
GET /blogs/:id/posts => listBlogPosts

@pvgoran
Copy link
Contributor Author

pvgoran commented Jan 20, 2019

I believe we need more comprehensive approach here. CRUDL helpers are good to start but often you start needing to replace them with regular definitions.

I don't understand what this has to do with this change. If a CRUDL definition is replaced with regular definitions, nothing prevents the programmer from manually specifying Operation IDs for individual definitions.

Also, I think we don’t need to consider the payload key name here, the last endpoint path member seems to be more relevant for the operation ID rather than the params format/schema.

I just followed the logic that is used to produce comments; I believe that Operation IDs should match entity names used in comments. The last endpoint path component is possibly more relevant for both of these uses (entity name for comments and entity name for Operation IDs), but it doesn't seem to be that strong preference to me.

In any case, my next PR allows specifying entity name (that is used for both the comments and the generated Operation IDs) manually, in a way similar to Operation IDs in normal definitions.

We could come up with a similar algorithm that would handle regular definitions too. For example like this:

POST /users => createUser
GET /blogs/:id/posts => listBlogPosts

It's an interesting idea in general, but it can easily go wrong. For example, I believe that combining several path components in one name can easily produce uncomprehensive results. Also, distinguishing between "list" and "retrieve" operations may be unreliable.

@Ajaxy
Copy link
Owner

Ajaxy commented Jan 24, 2019

I don't understand what this has to do with this change. If a CRUDL definition is replaced with regular definitions, nothing prevents the programmer from manually specifying Operation IDs for individual definitions.

If we only add this change for CRUDL endpoints it would be inconsistent with the format of all the other non-CRUDL endpoints, that now have (_get-users-id, _patch-companies-id etc). Plus it does not allow the user to change the format if he wants to. It means that we need to come up with some consistent format for the defaults (or none as we discussed in #13).

I believe that Operation IDs should match entity names used in comments.

I would change comments behavior too. I don't see any positive use case for a single custom key being different to the endpoint name.

It's an interesting idea in general, but it can easily go wrong. For example, I believe that combining several path components in one name can easily produce uncomprehensive results. Also, distinguishing between "list" and "retrieve" operations may be unreliable.

I believe that for properly written API using basic conventions we would cover a major amount of cases. For the rest now we have the support for custom operation IDs.

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