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

fixes issue #14 compiling on Win 7 now works #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dgollub
Copy link
Contributor

@dgollub dgollub commented Mar 7, 2016

This commit adds custom build.bat for Windows and changes the code so that it can be compiled on Windows.

The resulting exe files run without issues in Powershell, cmd.exe and Cygwin.

The code is not the cleanest and could be improved and there are also a few Notes and Todos throughout it that could be addressed in the future.

For now the big breakthrough is that "it compiles" on Windows.

Disclaimer: while I am working as professional developer I am rather new to D, so my code is likely not as idiomatic as it could be. Let me know if you need me to change anything. I'll see if I can make some time to improve things then. (Spent the last few nights on this, so hopefully this is not too bad.)

dgollub and others added 2 commits March 7, 2016 17:52
This commit adds custom build.bat for Windows and changes the code
so that it can be compiled on Windows.

The resulting exe files run without issues in Powershell, cmd.exe
and Cygwin.

The code is not the cleanest and could be improved and there are
also a few Notes and Todos throughout it that could be addressed
in the future.

For now the big breakthrough is that "it compiles" on Windows.
@mrkline
Copy link
Owner

mrkline commented Mar 8, 2016

Wow, thanks for all the work! I'll do my best to review it in the next day or two so we can get this merged.

@mrkline
Copy link
Owner

mrkline commented Mar 28, 2016

Sorry I was a liar! Life's been really busy in the past few weeks. Taking a look now...

assert(0); // force crash
}
s = pathConversion.output.strip();
} else {
Copy link
Owner

Choose a reason for hiding this comment

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

If we're not using this else, can we remove it?

@mrkline
Copy link
Owner

mrkline commented Mar 28, 2016

Along with the line-specific comments,

Trivial stuff:

  • All of promptoglyph-vcs.d is shown as a diff because the line endings have been converted to Windows ones (\r\n). Can you get them back to Unix ones? Same for .gitignore
  • I get that it was auto-generated, but can we pare down the .gitignore a bit? It went from 5 lines to 113, and it looks like a lot of it is unnecessary.
  • Lots of lines have trailing whitespace. Please remove it.
  • Indentation is messed up in asyncGetFlags().

(This is superficial stuff, but at the same time, it's also really easy to fix.)

Color

If color doesn't work in CMD, why provide some special case for it? Just trust the user to specify --nocolor. Also,

  1. One shouldn't assume Windows users are running this in CMD - things like mintty exist.
  2. Even if the above weren't the case, your changes seem to make color functions eat the string (instead of just returning it, unmodified) on Windows.

Misc:

  • The systempath bit is... unfortunate. Given the circumstances, I suppose that's the best we can do.
  • Instead of switching all the buildPath calls to customBuildPath, can we give the latter the same name and conditionally import one of the two depending on platform?

@dgollub
Copy link
Contributor Author

dgollub commented Jun 4, 2016

Hi @mrkline,

Sorry for the radio silence, but I was pretty busy with family and job.

Thank you so much for your code review and all your improvement suggestions. Those are greatly appreciated! Shows me that even after almost 20 years of programming, I am still making a lot of mistakes and I should never stop learning. :-)

I have a WIP with a lot of improvements (based on your suggestions/code review) over in my fork, but it is not quite ready yet. There are still some improvements I would like to add, like your "--no-color" suggestions for example.

Hopefully I'll find some time in the coming weeks to finish this. Thank you for your patience and again, thank you for the code review!

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

Successfully merging this pull request may close these issues.

2 participants