-
Notifications
You must be signed in to change notification settings - Fork 40
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
Properly render example
for array exposure done using another entity
#68
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! run rubocop -a ; rubocop --auto-gen-config
.
Danger error looks suspicious too. Wonder whether it has anything to do with -
at the end of the name.
This is so it matches waht happens in entity_model_type. The main difference is we use Symbols (both keys and values) here, whereas we use Strings there. I'm not sure the difference is intentional, but I didn't want to introduce any actual changes in this commit.
6955ed6
to
4ece68e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the problem in the CHANGELOG. Change -
to *
.
|
||
add_extension_documentation(entity_model_type, documentation) | ||
add_array_documentation(entity_model_type, documentation) if documentation[:is_array] | ||
return param unless (documentation = entity_options[:documentation]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The side-effect assignment of documentation
is too clever IMO. You should break it up.
CHANGELOG.md
Outdated
@@ -6,8 +6,9 @@ | |||
|
|||
#### Fixes | |||
|
|||
* [#67](https://github.com/ruby-grape/grape-swagger-entity/pull/67): Various build updates - [@mscrivo](https://github.com/mscrivo). | |||
* Your contribution here. | |||
- [#67](https://github.com/ruby-grape/grape-swagger-entity/pull/67): Various build updates - [@mscrivo](https://github.com/mscrivo). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes danger-changelog to break (fix in dblock/danger-changelog#63). These lines need to start with a *
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed that while committing, my editor was auto correcting these to -
, I went to quickly and thought the rest of the diff in this file (that I didn't commit) was whitespace changes 🤦🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It exposed a bug in danger-changelog, so I think it's a win!
The reason I'm opening this PR is because logic that was present for "standard" exposures was accidentally left out from "using: Entity..." exposures". The point of this commit is to hopefully prevent such omissions from being made again.
4ece68e
to
952de5e
Compare
One more complaint from RuboCop, run |
952de5e
to
8d9e1c5
Compare
For now I simply moved the |
This bug was introduced in ruby-grape#68 I've added a regression spec and a fix for this, along with a tiny bit of cleanup because there seem to have been some confusion in the naming of variables.
This bug was introduced in ruby-grape#68 I've added a regression spec and a fix for this, along with a tiny bit of cleanup because there seem to have been some confusion in the naming of variables.
This bug was introduced in ruby-grape#68 I've added a regression spec and a fix for this, along with a tiny bit of cleanup because there seem to have been some confusion in the naming of variables.
Right now the
example
documentation key is ignored. The first commit on its own fixes this, but then I opted to refactorAttributeParser#call
to make it harder to miss adding support for a documentation key for ausing
exposure.