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

Added folder browser (Windows only) and applied two Go conventions. #10

Merged
merged 1 commit into from
Jan 25, 2018
Merged

Added folder browser (Windows only) and applied two Go conventions. #10

merged 1 commit into from
Jan 25, 2018

Conversation

Schobers
Copy link
Contributor

I've implemented the browser dialog (Windows only). An example is provided with an explicit message that it is (for now?) only supported on Windows.

Secondly I've applied two Go conventions (errors starting with Err prefix and line comments instead of block comments).

@sqweek
Copy link
Owner

sqweek commented Jan 25, 2018

Looks cool, thanks for your work! I wondered for a moment if this should be a new spawning function on the File dialog, but that wouldn't really make sense in combination with Filter(), and dialog.Directory() is a nice clear entry point.

I think the gtk/osx implementations are fairly straightforward, so I'm happy to take them on and avoid the need to introduce the NotImplemented error at this stage.

I'm hesitant about the second commit. I appreciate you bringing the conventions to my attention; ideally I would have followed them to begin with. But at this point renaming Cancelled to ErrCancelled is a breaking API change, so I'd like to manage that separately with its own consideration rather than squeezing it in along with a new feature.

@Schobers
Copy link
Contributor Author

Ok, no problem, I didn't consider that the change would break the API. If you want I can create another commit that only contains the block to line comment changes.

About the new feature: do you want me to create another pull request with only the new feature so you can start working on the GTK/OSX implementation and removing the flag again? Or do you want to squeeze the Windows & GTK/OSX implementation in single commit yourself?

@sqweek
Copy link
Owner

sqweek commented Jan 25, 2018

About the new feature: do you want me to create another pull request with only the new feature so you can start working on the GTK/OSX implementation and removing the flag again?

That would be great, thanks. Feel free to have the GTK/OSX stubs panic.

Note that you also have the option of updating this PR, by force pushing to the master branch of your fork. But a new PR is fine too, whichever way you prefer :)

@Schobers
Copy link
Contributor Author

Done (I wasn't aware of how to update a PR). Note that I added the Title() method for DirectoryBuilder which was lacking in the previous commit.

@sqweek sqweek merged commit 1fb22e0 into sqweek:master Jan 25, 2018
sqweek added a commit that referenced this pull request Jan 25, 2018
@sqweek
Copy link
Owner

sqweek commented Jan 25, 2018

Thanks @Schobers! I've merged the directory browser in and added linux/osx support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants