Add api_type_parameters cop #146
Conversation
| @@ -0,0 +1,57 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| # rubocop:disable RSpec/ExampleLength | |||
There was a problem hiding this comment.
I think it's fine to move this to the .rubocop.yml config for the gem as it'll most likely raise for most cop specs in the future. wdyt?
| @@ -0,0 +1,55 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
We should also add it to the default.yml config as a disabled config (like https://github.com/boxt/boxt_rubocop/blob/main/config/default.yml#L10-L12).
Then in the apps that use the gem, we can set it to Enabled: true and only include the directories we're interested in (i.e. specific versions).
| def grape_api_class?(node) | ||
| node.each_ancestor(:class).any? do |ancestor| | ||
| ancestor.children.any? do |child| | ||
| child&.source == "Grape::API" |
There was a problem hiding this comment.
What about Grape entity classes? Do we want to ensure the types are defined correctly in there too? There may also be other directories that have Grape params declared (like helper classes). Is it possible to cover all of them in this cop?
There was a problem hiding this comment.
Ah good point. I've added support for Grape entities now (and refactored the way I was matching them too). Working on helpers now as well, but they might be a bit trickier.
There was a problem hiding this comment.
What if we ignored the class and just looked for param definitions without types? Then in the projects where we include the cop, we can set the directories we want to look inside explicitly using Include.
I took a similar approach with https://github.com/boxt/boxt_rubocop/blob/main/lib/rubocop/cop/boxt/api_path_format.rb, partly because I was struggling to detect a Grape API class (but now we can use parts of your solution to help with that).
Adds a custom cop that warns when a parameter is added to a Grape API endpoint without a type being set.
Description:
This adds a custom Rubocop cop that enforces the type attribute on all API parameters. I'll enable it for V2 and V3 of the API, as they're largely covered by types already, but will leave it disabled for V1 which isn't.
In addition to improving the documentation, one of the main reasons for adding this was so we could avoid an issue we had recently with some of the package endpoints where the type parameter had been configured incorrectly (e.g.
optional :foo, Stringrather thanoptional :foo, type: String), and then other developers had copied this syntax and it had spread from there.