Issue #30: SaveAttachment sets the Filename to the Full Path of the File #31
Conversation
…he Full Path of the File Added an optional argument to the SaveAttachment method: useAbsolutePathAsFileName = true
APIClient/Services/IServices.cs
Outdated
| /// <returns>Oid</returns> | ||
| Oid SaveAttachment(string filePath, Asset asset, string attachmentName); | ||
| Oid SaveAttachment(string filePath, Asset asset, string attachmentName, bool useAbsolutePathAsFileName = true); |
There was a problem hiding this comment.
Changing an existing signature is an interface-breaking change (even with default parameters, which don't do what you'd like them to do).
Instead, I suggest adding a new overload with the flag.
There was a problem hiding this comment.
Thanks @spazmodius! I'll add an overload and revert the changes to the interface. I initially thought to use an overload, but ended up with the optional argument. I appreciate the feedback!
Used spazmodius's adviceto avoid breaking the interface. Also fixed my mistake that caused the Mime type to be always be se to the same value.
|
Thanks again for the feedback. After some research, I have a better understanding of how I broke the interface and why changing the signature is a bad idea. I probably spent as much time coding as I did trying to name the overload argument in a way that describes the default functionality. |
|
I made it a point to gain a better understanding of interfaces over the weekend and I'm pretty sure that, while I correctly added the overload, I was wrong in adding the new overloaded method to the interface. My understanding is that this is basically the same problem as my original commit - it will break any other class that uses the interface (since they would be required to implement the new method). I believe that I should remove the additional method in the interface even though it doesn't seem to directly effect this project and I have a commit ready to go if I'm correct. I think where I was(am?) getting confused is by looking at the API Client as a whole. There's only one class that implements the IServices interface in the API Client, but it's possible that someone else using this project is borrowing the interface for their own needs, correct? In that case, my addition to the interface is still breaking the interface and I need to correct it. |
|
Adding an overload will maintain compatibility with clients that use the old signature. Implementers will have to update, of course, but as you've noted, there's just the one class; I can confirm that no other project implements The original method can now simply delegate to the new method. |
Based on Spazmodius's advice, I applied the DRY prinicple: don't repeat yourself.
|
Thanks for the advice @spazmodius ! Simplifying the original method makes a lot of sense - especially in regards to maintenance down the road (no need to make someone read through both methods when they are mostly the same). For myself and anyone who might come across this in the future, here's a blog post (albeit a dated one) regarding the caveats of using optional arguments like I was trying in my initial commit: https://lostechies.com/jimmybogard/2010/05/18/caveats-of-c-4-0-optional-parameters/ I appreciate your time and patience! |
|
Hi! I just wanted to check on this and see if there were any plans to merge this or close it. Thank you! |
See #30
Added an optional argument to the SaveAttachment method: useAbsolutePathAsFileName = true
When set to false, it will use the file name + extension. It defaults to true and uses the Absolute Path of the file passed in through the fileName argument.