Move 'sign_with_time' method to 'S3Action' #28
Conversation
|
Hey @paolobarbolini! |
Codecov Report
@@ Coverage Diff @@
## main #28 +/- ##
==========================================
+ Coverage 91.60% 91.84% +0.23%
==========================================
Files 25 26 +1
Lines 1286 1250 -36
==========================================
- Hits 1178 1148 -30
+ Misses 108 102 -6
Continue to review full report at Codecov.
|
z4f1r0v
force-pushed
the
mv-sign-with-time
branch
from
16c018a
to
609dfa4
Compare
|
@paolobarbolini on the first try I had some formatting errors. I have fixed those and resubmitted for review. |
|
@z4f1r0v why many functions are moved? 🤔 |
|
@guerinoni that's how I read the description of the issue I referenced above. Please correct me if I'm wrong. |
| /// Takes the time at which the URL should be signed | ||
| /// Used for testing purposes | ||
| fn sign_with_time(&self, expires_in: Duration, time: &OffsetDateTime) -> Url; |
There was a problem hiding this comment.
This refers to line 38 but I can't seem to be able to comment a line which hasn't been changed so I'll comment this one instead:
Could you add a default implementation of sign:
fn sign(&self, expires_in: Duration) -> Url {
let now = OffsetDateTime::now_utc();
self.sign_with_time(expires_in, &now)
}so that... (continues in the other comment)
| fn sign(&self, expires_in: Duration) -> Url { | ||
| let now = OffsetDateTime::now_utc(); | ||
| self.sign_with_time(expires_in, &now) | ||
| } |
There was a problem hiding this comment.
...you can remove all of these sign implementations which are all the same?
Basically if a trait has a default implementation for a certain function it'll use it when you do impl Trait for T unless you overwrite it with something else
https://doc.rust-lang.org/book/ch10-02-traits.html#default-implementations
There was a problem hiding this comment.
Of course. I'm familiar with the concept. I just did the brainless work of C&P :D
|
|
||
| .idea |
There was a problem hiding this comment.
Do we want this @guerinoni?
There was a problem hiding this comment.
Your repo - your rules. I can remove it if is a big problem.
There was a problem hiding this comment.
For me it's ok, maybe for who use this IDE can be handy
z4f1r0v
force-pushed
the
mv-sign-with-time
branch
from
609dfa4
to
94903a6
Compare
|
@paolobarbolini can this be merged? |
There was a problem hiding this comment.
@paolobarbolini can this be merged?
Yeah sorry I kept forgetting to do the final review. Thank you very much for the PR!
As per #27, this PR moves the the
sign_with_timeto theS3Actiontrait.