Add Active Job annotations #191
Conversation
rbi/annotations/activejob.rbi
Outdated
| sig do | ||
| type_parameters(:ExceptionType) | ||
| .params( | ||
| exceptions: T::Class[T.type_parameter(:ExceptionType)], | ||
| block: T.nilable( | ||
| T.proc.bind(T.attached_class).params( | ||
| job: T.attached_class, | ||
| error: T.type_parameter(:ExceptionType) | ||
| ).void | ||
| ) | ||
| ).void | ||
| end | ||
| sig do | ||
| params( | ||
| exceptions: String, | ||
| block: T.nilable(T.proc.bind(T.attached_class).params(job: T.attached_class, error: T.untyped).void), | ||
| ).void | ||
| end |
There was a problem hiding this comment.
Not sure if there's a policy on Sorbet versions supported. Can make this not use overloads like the methods below, but I wish Sorbet supported overloads with keyword args.
There was a problem hiding this comment.
👋 Hey Bart, it's been a while. Hope you're well! Jumping in here as it seems a bit off that discard_on is RBI typed differently than rescue_on and retry_on considering according to the Rails docs they're all the same.
- https://api.rubyonrails.org/classes/ActiveJob/Exceptions/ClassMethods.html#method-i-discard_on
- https://api.rubyonrails.org/classes/ActiveJob/Exceptions/ClassMethods.html#method-i-retry_on
- https://api.rubyonrails.org/classes/ActiveSupport/Rescuable/ClassMethods.html#method-i-rescue_from
Thoughts on bringing discard_on in-line with the others?
Bonus, would allow discard_on usage to avoid needing T.unsafe with the latest Sorbet provided the array of strings is frozen via freeze re: the following change.
There was a problem hiding this comment.
Hey @larouxn 😄 hope you're doing well too!
You're correct they're all the same but the discard_on sigs are more accurate when the first sig with generics applies. rescue_on and retry_on should ideally follow this example, except for the fact that we currently can't use overloads because of keyword arguments: sorbet/sorbet#37
There was a problem hiding this comment.
Understood. Assuming Sorbet keyword argument support is fixed, we should update all to use the more accurate overload style approach. Assuming support is not fixed, current state, does it make sense to bring discard_on in line with the other two? In the end, I'm thinking consistency either way would be best, leaning towards non-overload given we don't know when if ever kwarg support will be fixed. 😅
There was a problem hiding this comment.
does it make sense to bring
discard_onin line with the other two?
I'm not following the "would allow discard_on usage to avoid needing T.unsafe" argument. Can you help me show exactly what you mean?
This is what I got right now with your proposed changes, it doesn't seem different for stringified error classes:
https://sorbet.run/#%23%20typed%3A%20true%0A%0Aclass%20FooJob%0A%20%20extend%20T%3A%3ASig%0A%20%20%0A%20%20sig%20do%0A%20%20%20%20params%28exceptions%3A%20T.any%28Module%2C%20String%29%2C%20block%3A%20T.nilable%28T.proc.params%28job%3A%20T.attached_class%2C%20error%3A%20T.untyped%29.void%29%29.void%0A%20%20end%0A%20%20def%20self.discard_on%28*exceptions%2C%20%26block%29%3B%20end%0A%0A%20%20DISCARD_ERRORS%20%3D%20%5B%22Bar%22%2C%20%22Baz%22%5D.freeze%0A%0A%20%20discard_on%28*DISCARD_ERRORS%29%20do%20%7Cjob%2C%20error%7C%0A%20%20%20%20T.reveal_type%28error%29%0A%20%20end%0Aend
There was a problem hiding this comment.
Thanks for taking a look! Hmm, I thought (could be wrong) that making discard_on typed the same as rescue_on and retry_on would make working with frozen arrays of string better. I could be wrong.
For a slight bit more context, in my experience working with latest Sorbet, as of sorbet/sorbet#7993, splatting a frozen array of strings with rescue_on and retry_on is fine, no errors but doing the same with discard_on will lead to an error due to the stricter expectation that all string/non-string classes passed to it are of ExceptionType.
Expected T::Class[T.type_parameter(:ExceptionType)] but found T.class_of(Permanent) for argument exceptions https://srb.help/7002Thus, it's not a bug, it's expected behaviour. But at the same time, I think it would be clearer if all three ActiveJob methods had the same typing/expectations.
There was a problem hiding this comment.
Update, seemingly good news, appears we can't leave ExceptionType as is and update the signature below from exceptions → String to T.any(Module, String), like the other two methods. #263 (comment) 🙌
|
I'll run this against Shopify core to verify. |
|
Verified with: 2 new typecheck failures, but just needed a |
Thanks for calling that out. E.g. when using |
bdewater
force-pushed
the
aj-class-methods
branch
from
6ad0532
to
719d6aa
Compare
bdewater
force-pushed
the
aj-class-methods
branch
from
719d6aa
to
93391ff
Compare
|
All good now on Shopify core. |
| sig do | ||
| params( | ||
| exceptions: T.any(Module, String), | ||
| wait: T.any(ActiveSupport::Duration, Integer, Symbol, T.proc.params(executions: Integer).returns(Integer)), |
There was a problem hiding this comment.
🤔 I don't see ActiveSupport::Duration in the docs, even though the implementation indicates it should work.
There was a problem hiding this comment.
3.seconds is the default value which is a duration? :)
There was a problem hiding this comment.
Yeah, and I now see the examples also use a duration.
| sig { params(part_name: T.nilable(T.any(String, Symbol)), block: T.nilable(T.proc.bind(T.attached_class).returns(T.untyped))).void } | ||
| def self.queue_as(part_name = nil, &block); end | ||
|
|
||
| sig { params(priority: T.untyped, block: T.nilable(T.proc.bind(T.attached_class).returns(T.untyped))).void } |
There was a problem hiding this comment.
Isn't priority always an Integer?
There was a problem hiding this comment.
Honestly I had no idea, I've never used this feature TBH and I wasn't sure if this was backend-specific. Can make the change.
There was a problem hiding this comment.
It seems there's not much documentation or examples for that, so let's keep it as T.untyped.
|
Thanks @bdewater! |
Type of Change
Changes