-
Notifications
You must be signed in to change notification settings - Fork 552
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
Add support for ScheduledQueryRun #666
Conversation
|
||
# Offense count: 1 | ||
# Configuration parameters: CountComments. | ||
Metrics/ModuleLength: | ||
Max: 314 | ||
Max: 315 |
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.
Those changes are getting a little bit annoying. We're basically incrementing each time and making those rules irrelevant. Wanted to flag as we likely should disable the rule for the 2 specific parts of code we care about.
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.
Yeah, we should just kill these. I can do it.
@@ -38,8 +38,6 @@ class MainResource < APIResource | |||
OBJECT_NAME = "mainresource".freeze | |||
nested_resource_class_methods :nested, | |||
operations: %i[create retrieve update delete list] | |||
|
|||
OBJECT_NAME = "mainresource".freeze |
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.
Noticed a weird warning today. This likely should have been caught by the test suite though so it's worth investigating what caused it to not be seen though it's just in tests.
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.
Arg yes, I've run into this type of thing before.
Unfortunately, there doesn't seem to be any easy way to have Ruby treat warnings as errors (I don't really want to monkeypatch Kernel
although I suppose we could consider it).
These don't come up that often though, so I'm not sure it's a huge problem.
@@ -77,6 +77,7 @@ | |||
require "stripe/recipient_transfer" | |||
require "stripe/refund" | |||
require "stripe/reversal" | |||
require "stripe/sigma/scheduled_query_run" |
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.
Kind of nice/lucky that this wasn't brought in until you'd established the new namespacing convention.
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.
full disclosure, I saw that resource once and tried on a week end, and failed and thought "well no one has asked for it so it's okay" 😂
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.
lol! Well, pretty nice that it's a piece of cake nowadays right?
Thanks Remi! LGTM. |
Released as 3.19.0. |
Remi pointed out in #666 that we basically just have to keep adding more more onto the `Max` exception for both these rules every time we add a new API resource. Here I suggest that we modify the check on method length in two ways: 1. Permanently disable the cop on `Util.object_classes`. This is just going to keep growing until we change are approach to it. 2. Choose a more reasonable maximum of 50 lines for elsewhere (IMO, the default of 10 is just too short). Most of our methods already come in below this, but there's a couple outliers like `#execute_request` in `StripeClient`. If we knock over some of those, we could lower this number again, but I suspect that we'd probably want somewhere closer to 30 (instead of 10) event then. I also disable the check on module length completely. I'm not convinced this is a very good heuristic for code quality.
r? @brandur-stripe
cc @stripe/api-libraries