[Proposal] Update Swagger's definition keys (more verbose) #1207
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
IIRC they should be unique |
|
They are! |
soyuka
approved these changes
Jun 27, 2017
|
LGTM but I want to have @dunglas's confirmation on this. Could you rebase thought ? |
Simperfit
approved these changes
Jun 27, 2017
dunglas
reviewed
Jun 27, 2017
| } else { | ||
| $definitionKey = $resourceMetadata->getShortName(); | ||
| } | ||
| $definitionKey = $this->getDefinitionKey($resourceMetadata->getShortName(), (array) ($serializerContext['groups'] ?? null)); |
There was a problem hiding this comment.
$serializerContext['groups'] ?? []?
dunglas
requested changes
Jun 27, 2017
| * @param string $resourceShortName | ||
| * @param array $groups | ||
| * | ||
| * @return string |
There was a problem hiding this comment.
Useless docblock, can be removed
| */ | ||
| private function getDefinitionKey(string $resourceShortName, array $groups): string | ||
| { | ||
| return !$groups ? $resourceShortName : $resourceShortName.'_groups_'.implode('_', $groups); |
There was a problem hiding this comment.
I suggest the following to avoid a useless negation: $groups ? sprintf('%s-%s', $resourceShortName, implode('_', $groups)) : $resourceShortName
I also suggest to replace the _groups_ part by a single dash (less verbose).
meyerbaptiste
force-pushed
the
update_definition_key
branch
from
ccb7b0b
to
1176a45
Compare
meyerbaptiste
force-pushed
the
update_definition_key
branch
from
1176a45
to
ec1a19d
Compare
|
Changes are done. |
|
WTF the coverage, men this drives me nuts. We may wanna think about changing the coverage service... |
dunglas
approved these changes
Jun 29, 2017
|
Thank you @meyerbaptiste! |
hoangnd25
pushed a commit
to hoangnd25/core
that referenced
this pull request
Feb 23, 2018
…tion_key [Proposal] Update Swagger's definition keys (more verbose)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since Swagger-UI 3.x, definitions are exposed in the ui. My proposal is to make them more verbose when an operation has defined roles. So instead of a md5 hash, I propose to use a slug.
Slug can be discussed and currently I use
_groups_<group x>_<group y>_<group ...>.Before:
After:
WDYT?