Conversation
Sorry for going back and forth. I have updated the original winattr. If we wait for that a little, we can use the promisfied library without adding extra code. |
@aminya no worries. I think it might be worth merging this anyways, though. Your change is a breaking change and I don't expect the author to merge it soon. But who knows :P I can always revert this commit if needed, so no big deal. Let's see what the maintainers say. |
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.
Looks great. Just 2 comments.
@@ -2601,4 +2614,18 @@ class SearchCallbackArgument { | |||
} | |||
} | |||
|
|||
let _winattr = null |
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.
Do we really need this here?
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.
Do you want it to live somewhere else in the file? Maybe at the top? I added it there because in order to understand the getPromisifiedWinattr
function you can just look at that piece of code and understand it all, no need to scroll to know what _winattr
is. But I'm happy to move it somewhere else to follow Atom's conventions.
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.
I wasn't getting the idea behind defining it there. But I think I see the point now. Correct me if am work. It is defined here so that calling getPromisifiedWinattr
multiple times does not result in multiple require
calls.
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.
Yeah that's right. It will also only promisify the methods once.
I think you should go ahead and merge this. I will create a new promisified |
Thanks for the review @sadick254! I addressed your comments. |
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.
Looks great. 👍
Identify the Bug
This PR addresses atom/atom#13538
Description of the Change
Trying to save a hidden file in Windows will raise a permission error. This PR uses winattr to bypass that issue by toggling the hidden attribute to false while saving, and then turning it back on when done.
Alternate Designs
I do introduce a dependency (winattr) as there's no easy way to handle attributes for files in Windows using Node.js. Alternatively, it could be written by hand, but it's a lot of tests and code that is better delegated to it's own library. Winattr is quite popular and stable so I think it's by far the best option.
Possible Drawbacks
--
Verification Process
I tried it locally on my Windows 10 installation and it worked. I also added tests and they passed.
Release Notes
TextBuffer is now able to save hidden files in Windows