Add support for repetition and ISO 8601 for reminders - #974
Conversation
Codecov Report
@@ Coverage Diff @@
## master #974 +/- ##
==========================================
- Coverage 70.17% 70.16% -0.01%
==========================================
Files 160 160
Lines 5284 5377 +93
Branches 567 583 +16
==========================================
+ Hits 3708 3773 +65
- Misses 1442 1464 +22
- Partials 134 140 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
78f6748 to
9cd3edf
Compare
yash-nisar
changed the title
halspang
left a comment
halspang
left a comment
There was a problem hiding this comment.
This is a change that I'd also feel better about if we had e2e tests. When we change how a data type like this goes to the runtime, it's always good to validate that the runtime does in fact understand it.
There was a problem hiding this comment.
This is because IDaprInteractors was not accessible.
Error: Dapr.Actors.Runtime.DefaultActorTimerManagerTests.RegisterReminderAsync_WithRepetition_CallsInteractor_WithCorrectData: Castle.DynamicProxy.Generators.GeneratorException : Can not create proxy for type Dapr.Actors.IDaprInteractor because it is not accessible. Make it public, or internal and mark your assembly with [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7")] attribute, because assembly Dapr.Actors is strong-named.
There was a problem hiding this comment.
I see you did this for the test. I'd argue that you should just make a small implementation of the interface instead of changing this property.
There was a problem hiding this comment.
Thoughts? I'm still not convinced we need this and I don't want to change the visibility like this.
There was a problem hiding this comment.
The only other option is to make the visibility of the interactor public which will cause a domino effect will lead to inconsistent accessibility issue. The param ActorMessageSerializersManager will also have to be public, etc and the chain would continue. Do you know if there is a better way to mock the interactor ?
There was a problem hiding this comment.
Yes, implement it :) You're only doing one method so just create a stub implementation that does what your mock does.
d51cdd2 to
9d04f3e
Compare
|
The IT test failure is unrelated to my change. |
There was a problem hiding this comment.
I know you got this from above, but is -1 even a valid period? I think an empty period is actually correct and -1 may be invalid.
https://docs.dapr.io/reference/api/actors_api/#create-actor-reminder
There was a problem hiding this comment.
Just saw that we're accepting values > 0 for repetitions. So, getting rid of that statement.
There was a problem hiding this comment.
This statement feels odd to me. -1 is a special case I'm guessing? Would we be calling it with that at any time instead of just not setting it? Couldn't we just say anything <0 is a valid way to say infinite repetitions?
There was a problem hiding this comment.
Exception should include the value range since we know it.
There was a problem hiding this comment.
This should be optional, yeah? int?
There was a problem hiding this comment.
I see you did this for the test. I'd argue that you should just make a small implementation of the interface instead of changing this property.
There was a problem hiding this comment.
The code probably uses System.Text.Json instead of Newtonsoft, you should as well to ensure we don't have any weird serializer issues.
There was a problem hiding this comment.
Period and due time are booleans?
There was a problem hiding this comment.
data.TryGetValue() returns a boolean stating whether the property exists in the json object or not. But I have changed it anyway.
267400b to
2eb3bcc
Compare
|
Test failure is unrelated to my change. |
There was a problem hiding this comment.
if (repetitions != null && repetitions <= 0) {
...
}Feels like we need that ^.
There was a problem hiding this comment.
Comment should include optional like Ttl above.
There was a problem hiding this comment.
Thoughts? I'm still not convinced we need this and I don't want to change the visibility like this.
There was a problem hiding this comment.
Should've asked this earlier, but how do ttl and repetitions interact? Is it OK to have both?
There was a problem hiding this comment.
A very good catch, on reading the docs, I found that it is OK to have both. Adding a method overload.
There was a problem hiding this comment.
Can this not be default valued? I know we can set it as null but we are forcing the set.
2eb3bcc to
fc2c1de
Compare
halspang
left a comment
halspang
left a comment
There was a problem hiding this comment.
Main thing is to switch around some stuff in the tests.
There was a problem hiding this comment.
Seems like this shouldn't be here.
There was a problem hiding this comment.
If this is for testing, it should be in the testing project otherwise this is included in all projects that use the dapr libraries.
There was a problem hiding this comment.
If this was also for the test, I think we should remove it. I'd prefer that interface exist in the test project instead of being included in the main library.
There was a problem hiding this comment.
Yeah, it's here where I was saying a simple impl of the interface would mean we don't need to use a mock at all.
There was a problem hiding this comment.
As with the TTL test, we should check that things don't change after we expect the reminder to be gone.
Signed-off-by: Yash Nisar <yashnisar@microsoft.com>
fc2c1de to
c8fd5a9
Compare
2 participants
Closes #702
Signed-off-by: Yash Nisar yashnisar@microsoft.com
Description
Add support for reptition and ISO 8601 for reminders
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #702
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: