-
Notifications
You must be signed in to change notification settings - Fork 41
Remove hidden attributes from required params #87
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
Conversation
numbata
left a comment
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 for the fix! I ran into this same issue yesterday and started working on a fix on the grape-swagger side as well. I think both fixes deserve to exist - yours handles it properly at the grape-swagger-entity side (where the root cause is), while the grape-swagger fix will act as a safeguard for similar issues that might come from other grape-swagger-* gems, not just grape-swagger-entity. Merging!
|
@bogdan Can you update CHANGELOG.md ? |
- Update `required_params` method to skip hidden attributes when determining required parameters. - Modify `response_model_spec` to reflect changes in required attributes, removing `hidden_attr`. - Add a test in `parser_spec` to ensure hidden attributes are not marked as required.
|
@dblock Hi! I found that the GitHub token hardcoded in the Danger workflows (in grape-swagger-entity and other grape repos) has expired. The token is returning |
|
@numbata I opened ruby-grape/grape#2630 with some details, feel free to pick it up! In the meantime removing danger or ignoring it is probably the easiest way, but I'd prefer a real fix. |
|
@numbata any change merging this without ruby-grape/grape#2630 being resolved? |
|
@bogdan I plan to fix the Danger issue in the repo today (it just needs a GitHub Actions workflow update), and then we can move forward with this PR. |
|
@bogdan can you rebase from |
Danger ReportNo issues found. |
|
Let's fix a little, so I can merge it? diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 78d143c..67cc731 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -37,7 +37,7 @@ Metrics/AbcSize:
# Offense count: 2
# Configuration parameters: CountComments, CountAsOne.
Metrics/ClassLength:
- Max: 117
+ Max: 120
# Offense count: 2
# Configuration parameters: AllowedMethods, AllowedPatterns.
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 66235e1..1044d41 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,7 +7,7 @@
#### Fixes
* Your contribution here.
-* [#87](https://github.com/ruby-grape/grape-swagger-entity/pull/87): remove hidden attributes from required [@bogdan](https://github.com/bogdan)
+* [#87](https://github.com/ruby-grape/grape-swagger-entity/pull/87): remove hidden attributes from required - [@bogdan](https://github.com/bogdan).
* [#88](https://github.com/ruby-grape/grape-swagger-entity/pull/88): Update danger workflows - [@numbata](https://github.com/numbata).
### 0.7.0 (2025/08/02) |
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@dblock.org>
Increased maximum class length from 117 to 120.
After update to grape 3.0, hidden attributes started to appear inside required params. I am not sure why hidden parameters were not required when the same version of
grape-swagger-entitywas used with earlier versions ofgrape-swagger. But I think it requires a fix anyway.The bug was likely introduced here: b2a7b90
cc @numbata