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

Add short Microsoft Windows section #361

Merged
merged 4 commits into from Feb 17, 2016
Merged

Add short Microsoft Windows section #361

merged 4 commits into from Feb 17, 2016

Conversation

@dspinellis
Copy link
Contributor

@dspinellis dspinellis commented Jan 26, 2016

No description provided.

@jlevy
Copy link
Owner

@jlevy jlevy commented Jan 26, 2016

Welcome addition, thanks! Definitely willing to include a section like this.

How about we call it "Windows only"? I'd like to clarify that other items may apply to Windows, but that the OS X and Windows sections are for items that only work on that platform.

Secondly, I'm not a Windows user so would appreciate any review/feedback on this section from one or two more die-hard Windows folks!

@dspinellis
Copy link
Contributor Author

@dspinellis dspinellis commented Jan 26, 2016

OK, I agree with a "Windows only" title. It matches the OS X section. I didn't use it, because the section was broader, proposing the use of Cygwin.

@cstanhope
Copy link

@cstanhope cstanhope commented Jan 27, 2016

I use Windows and Cygwin daily, so I hope my review will be useful. I took a quick glance through the rest of the document, and I don't see anything that doesn't also apply to Cygwin. So I think it is safe to include the Cygwin specific section. I have two suggestions:

  1. Add a link to the Cygwin site. I know people can search for it, but sometimes searching can lead people astray. A direct link would avoid that. (Although I realize the OS X section doesn't link to homebrew.)
  2. Perhaps a quick warning that under Cygwin there is a mapping of the posix filesystem to the Windows system. New users may want to see the documentation for an explanation.

But even without those changes, I think this an excellent addition to the document. I even learned something (about /dev/clipboard) even though I've been using Cygwin for years!

@jlevy
Copy link
Owner

@jlevy jlevy commented Jan 27, 2016

@cstanhope thanks for the feedback.

Links are very welcome. The lack of links is entirely due to laziness on my part. I can think of no reason to avoid (helpful and stable) links. More should be present for OS X, Windows, and throughout the doc, actually.

Addresses: #issuecomment-175369010
@dspinellis
Copy link
Contributor Author

@dspinellis dspinellis commented Jan 27, 2016

@jlevy Thank you for the clarification regarding links. I was wondering whether this was a matter of style.

Addresses: #issuecomment-175369010
@jlevy
Copy link
Owner

@jlevy jlevy commented Jan 27, 2016

Great. Since we've requested feedback, I'll leave this one open for another day in case there is anything else, and then merge in.

@JasonCoombs
Copy link

@JasonCoombs JasonCoombs commented Jan 28, 2016

I'd like to review and comment but need a few days, if you'll leave this open a little longer. Thanks.

@dspinellis
Copy link
Contributor Author

@dspinellis dspinellis commented Jan 28, 2016

@cstanhope Thank you for the feedback. I incorporated the changes you suggested in 448887b.
@JasonCoombs Thank you for volunteering. I'm happy to wait.

@cstanhope
Copy link

@cstanhope cstanhope commented Jan 28, 2016

@dspinellis Your changes address my suggestions. Looks good!

@jlevy
Copy link
Owner

@jlevy jlevy commented Feb 3, 2016

@JasonCoombs will you still review?

@JasonCoombs
Copy link

@JasonCoombs JasonCoombs commented Feb 4, 2016

Working on a write-up of suggestions today. Thanks for your patience.

@JasonCoombs
Copy link

@JasonCoombs JasonCoombs commented Feb 16, 2016

I'm still working on this, but please everyone see my request for discussion, here: #165 (comment)

jlevy added a commit that referenced this pull request Feb 17, 2016
Add short Microsoft Windows section
@jlevy jlevy merged commit c3869fe into jlevy:master Feb 17, 2016
@jlevy
Copy link
Owner

@jlevy jlevy commented Feb 17, 2016

Thanks for the discussion (will follow up more there shortly).

I believe this is a solid addition, and doesn't preclude expansion or improvement, so have merged it in.

Thank you @dspinellis ! And @JasonCoombs and @cstanhope for the comments.

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

Successfully merging this pull request may close these issues.

None yet

4 participants