Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Windows #14

Merged
merged 5 commits into from Feb 21, 2017
Merged

Support Windows #14

merged 5 commits into from Feb 21, 2017

Conversation

mattn
Copy link
Contributor

@mattn mattn commented Feb 3, 2017

Just separated codes

@mattn
Copy link
Contributor Author

mattn commented Feb 3, 2017

Ah, should i make p-r from develop branch again?

@metalmatze metalmatze added this to the 1.1.0 milestone Feb 3, 2017
@metalmatze metalmatze added feature Enhancements and new features help-wanted labels Feb 3, 2017
@metalmatze
Copy link
Contributor

Yep. This PR should definitely target develop 😊 👍
Thanks for taking the time for this PR!

@mattn
Copy link
Contributor Author

mattn commented Feb 3, 2017

updated base of this p-r.

@dominikschulz dominikschulz changed the base branch from develop to master February 3, 2017 15:51
@dominikschulz dominikschulz reopened this Feb 3, 2017
Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a look at the color lib and the fsutil split.

action/init.go Outdated
@@ -5,9 +5,14 @@ import (

"github.com/fatih/color"
"github.com/justwatchcom/gopass/gpg"
"github.com/mattn/go-colorable"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we'd need another color library along the existing one. If you believe your lib provides a benefit over the existing one feel free to open an separate PR to completely replace the old one, but for this PR I'd prefer if we could stick to the existing one.


// Tempdir returns a temporary directory suiteable for sensitive data. On
// Windows, just return empty string for ioutil.TempFile.
func Tempdir() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Tempdir function seems to be the only difference between fsutil_windows.go and fsutil_others.go.

Could you leave the common code in fsutil.go and only put the Tempdir function in fsutil_windows.go / fsutil_others.go?

@dominikschulz
Copy link
Member

Very nice PR overall, thanks so much.

@dominikschulz dominikschulz removed the request for review from metalmatze February 4, 2017 13:50
@dominikschulz dominikschulz self-assigned this Feb 4, 2017
@mattn
Copy link
Contributor Author

mattn commented Feb 6, 2017

removed out variable. This was remaining to output color with color.GreenString(). It is not required by color.Green(). Also separated funcs in fsutil.

@bbigras
Copy link
Contributor

bbigras commented Feb 20, 2017

Any progress on this? I'm looking forward for this feature :).

@mattn
Copy link
Contributor Author

mattn commented Feb 21, 2017

This is already done. :)

@bbigras
Copy link
Contributor

bbigras commented Feb 21, 2017

This is already done. :)

Can't wait for it to be merged then.

@mattn
Copy link
Contributor Author

mattn commented Feb 21, 2017

rebased to master

@dominikschulz
Copy link
Member

dominikschulz commented Feb 21, 2017

We'll try to get this one merged ASAP.

Copy link
Member

@dominikschulz dominikschulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks really good.

@dominikschulz dominikschulz merged commit be6ed5b into gopasspw:master Feb 21, 2017
@mattn mattn deleted the windows branch February 21, 2017 17:22
@bbigras bbigras mentioned this pull request Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Enhancements and new features help-wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants