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 for windows platform #42

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Conversation

wizioo
Copy link

@wizioo wizioo commented Mar 12, 2014

As you askes me, here's my version working on windows.
I was a little surprised by the develop branch having a totally different structure from master where I originally changed the code.
I have 2 points I'd want you to see:

  • Try dbDump. I don't know if it still works on Linux without backslashes in cmd
  • I don't know what to return in dbReplace() as there's no more cmd to return. Now I use shelljs sed command... so I thought you could return shell but I don't know what to do with unit testing.

I hope my contribution is useful and usable for you (and others).

However, thank you for this fantastic grunt plugin!

wizioo added 2 commits March 12, 2014 09:43
…atform

- Also remove search_replace template as it is now unuses
- dBReplace() function now return shell and not cmd anymore.
    Unit test will change as shell.sed returns the new string after replacement
    (But I'm not at ease w/ unit testing so I'll let this part to another).
- I don't know if it still work on other platforms
@getdave
Copy link
Owner

getdave commented Mar 12, 2014

@wizioo thanks for this pull request. It is very much appreciated. I will review just as soon as I can.

@nickbouton
Copy link

@wizioo @getdave Can you confirm that this properly replaces on Win7? I'm still having trouble - it's only replacing the first instance of the URL from what I'm seeing.

@getdave
Copy link
Owner

getdave commented Mar 24, 2014

@nickbouton I can't confirm this I'm afraid as I don't have a windows machine. Sorry about that. Maybe @wizioo can help you there?

Just so you're both aware my plan for ongoing development to counter this issue would be to provide a option in the grunt task config which allows the user to supply their own search/replace mechanism. This would allow Windows users to create and test their own search/replace without me needing to buy and test on the windows machine.

I also feel such an approach would be the most extendible. Any thoughts?

@wizioo
Copy link
Author

wizioo commented Apr 15, 2014

@nickbouton You're right... sed command only replace the 1st instance of the url.
We have to find another solution. Any idea for the "find and replace" on windows machines?

PS: Sorry for not having answered before, I didn't have time.

@nickbouton
Copy link

@wizioo Maybe using powershell instead? Not sure offhand.

http://stackoverflow.com/questions/60034/how-can-you-find-and-replace-text-in-a-file-using-the-windows-command-line-envir
http://stackoverflow.com/questions/20077763/execute-powershell-script-from-grunt

Might be some way of getting it to work via shell.sed() as well. Not sure why it's only replacing one instance other than missing some parameters in addition to -i. The -r switch allows usage of extended regular expression syntax (on Windows at least). Maybe that's needed?

@wizioo
Copy link
Author

wizioo commented Apr 16, 2014

@nickbouton I think I've got it!
shell.sed() hasn't "global" option. I had a look on powershell but it was too restrictive.
But I finally just used js own search and replace functions. It would work on any OS. I only had to read an write the file, and Grunt know how to do this.

Just use it in db_replace instead of sed command:

var reg= new RegExp(search,"g");               //Create a global regexp w/ search string 
var myFile=grunt.file.read(output_file);        //Get file as a string
var result = myFile.replace(reg, replace);    //Replace search string by replace string
grunt.file.write(output_file, result);                //Write in file

I'll do a pull request w/ this feature as soon as possible.

@wizioo
Copy link
Author

wizioo commented Apr 18, 2014

Hi there,
I've tested and it seems to work on windows. No reason that it doesn't on other platform.
It would be better to have this grunt plugin to be universal by default rather than having to configure your gruntfile w/ different options according to your platform.

@nickbouton Can you test it and confirm ?

@getdave I hope it will be included on your next release. Thanks for the work already done.

@nickbouton
Copy link

Thanks for your work on this, @wizioo. I will test it out this evening and get back to you.

@getdave
Copy link
Owner

getdave commented Apr 28, 2014

Guys if this is just to do with relying on the use of sed to control search and replace then you might like to look at this branch

https://github.com/getdave/grunt-deployments/tree/feature/advanced-search-replace

More here

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.

3 participants