-
Notifications
You must be signed in to change notification settings - Fork 82
Set modified date in EmbeddedFileProvider #210
Conversation
// REVIEW: Does this even make sense? | ||
_lastModified = DateTimeOffset.MaxValue; | ||
|
||
_lastModified = DateTimeOffset.Now; |
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.
UtcNow
?
🆙 📅 |
71101dc
to
e5b3f1c
Compare
#if NETSTANDARD1_5 | ||
if (_assembly.Location != null) | ||
{ | ||
_lastModified = File.GetLastWriteTimeUtc(_assembly.Location); |
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.
This can throw a lot of exceptions that we may be better off suppressing.
https://msdn.microsoft.com/en-us/library/system.io.file.getlastwritetimeutc(v=vs.110).aspx
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.
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.
👍
The location of the loaded file that contains the manifest. If the loaded file was shadow-copied, the location is that of the file after being shadow-copied. If the assembly is loaded from a byte array, such as when using the Load(Byte[]) method overload, the value returned is an empty string ("").
Lots of edge cases.
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.
No harm in try catching this? On a bit of a tangent, did we consider having the alternative where the user specifies this value? It removes a bunch of this guessing from our code (plus the oddity with having different behavior based on what you might be targeting)?
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.
That's not a bad cop-out..
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.
We considered it, but weren't huge fans
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.
casual fans?
1 similar comment
🆙 📅 |
e5a0e0f
to
2690da8
Compare
2690da8
to
a5366af
Compare
#208
Apps that target
>=net462
ornetcoreapp1.0
will get the new behaviorOther targets will get the
DateTimeOffset.Now
behavior@Eilon @Tratcher @pakrym