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

Properly render example for array exposure done using another entity #68

Merged
merged 4 commits into from
Apr 14, 2024

Conversation

magni-
Copy link
Contributor

@magni- magni- commented Apr 12, 2024

Right now the example documentation key is ignored. The first commit on its own fixes this, but then I opted to refactor AttributeParser#call to make it harder to miss adding support for a documentation key for a using exposure.

Copy link
Member

@dblock dblock left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
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.
Copy link
Member

@dblock dblock left a 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])
Copy link
Member

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).
Copy link
Member

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 *.

Copy link
Contributor Author

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 🤦🏼

Copy link
Member

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.
@dblock
Copy link
Member

dblock commented Apr 13, 2024

One more complaint from RuboCop, run rubocop -a ; rubocop --auto-gen-config.

@magni-
Copy link
Contributor Author

magni- commented Apr 13, 2024

One more complaint from RuboCop, run rubocop -a ; rubocop --auto-gen-config.

Metrics/ClassLength: Class has too many lines. [101/100]

For now I simply moved the if ... test in the assignment to param at the start of #call to be on the same line (matching the existing style used in #document_data_type)

@dblock dblock merged commit 2615fad into ruby-grape:master Apr 14, 2024
6 checks passed
Jell added a commit to Jell/grape-swagger-entity that referenced this pull request Apr 29, 2024
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.
Jell added a commit to Jell/grape-swagger-entity that referenced this pull request Apr 29, 2024
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.
Jell added a commit to Jell/grape-swagger-entity that referenced this pull request May 4, 2024
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.
@magni- magni- deleted the pp/fix-array-example branch May 20, 2024 04:25
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.

None yet

2 participants