|
|
Descriptioncodereview: use subprocess.call() instead of os.spawnvp() for portability
Fixes issue 4121.
Patch Set 1 : diff -r 90c9121e26c3 https://code.google.com/p/go #
MessagesTotal messages: 12
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
the change LGTM. On Saturday, September 22, 2012, wrote: > > Description: > codereview.py : uses os.spawnvp() for spwaning gofmt - does not work on > windows I think this is better: codereview: use subprocess.call() instead of os.spawnvp() for portability > > This is not portable across different OSes. subprocess.call() is used now instead to fix the issue. Then you do not need these two lines. > > Fixes issue 4121. >
Sign in to reply to this message.
What is the procedure to change the description text that is already submitted to codereview? On 2012/09/22 04:44:08, minux wrote: > the change LGTM. > > On Saturday, September 22, 2012, wrote: > > > > Description: > > codereview.py : uses os.spawnvp() for spwaning gofmt - does not work on > > windows > > I think this is better: > codereview: use subprocess.call() instead of os.spawnvp() for portability > > > > > This is not portable across different OSes. > > subprocess.call() is used now instead to fix the issue. > > Then you do not need these two lines. > > > > > Fixes issue 4121. > >
Sign in to reply to this message.
hg change 6555049 will let you edit the CL description then hg mail 6555049 http://golang.org/doc/contribute.html#tmp_65 On Sat, Sep 22, 2012 at 9:43 PM, <shivakumar.gn@gmail.com> wrote: > What is the procedure to change the description text that is already > submitted to codereview? > > > > On 2012/09/22 04:44:08, minux wrote: >> >> the change LGTM. > > >> On Saturday, September 22, 2012, wrote: >> > >> > Description: >> > codereview.py : uses os.spawnvp() for spwaning gofmt - does not work > > on >> >> > windows > > >> I think this is better: >> codereview: use subprocess.call() instead of os.spawnvp() for > > portability > >> > >> > This is not portable across different OSes. > > >> subprocess.call() is used now instead to fix the issue. > > >> Then you do not need these two lines. > > >> > >> > Fixes issue 4121. >> > > > > > > http://codereview.appspot.com/6555049/
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, minux.ma@gmail.com, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2012/09/22 11:59:08, dfc wrote: > hg change 6555049 > > will let you edit the CL description > > then hg mail 6555049 > > http://golang.org/doc/contribute.html#tmp_65 > Unfortunately this hint does not work if the change is *only* for commit description. The tool ended up creating empty updates to the code review list for each attempt and crashing mercurial in the process - another bug :( Finally managed to do so with some work-arounds.
Sign in to reply to this message.
> Unfortunately this hint does not work if the change is *only* for commit > description. > The tool ended up creating empty updates to the code review list for > each attempt and crashing mercurial in the process - another bug :( > Finally managed to do so with some work-arounds. Please raise a bug for this, hg change works on *nix systems.
Sign in to reply to this message.
On 2012/09/22 13:51:28, dfc wrote: > > Please raise a bug for this, hg change works on *nix systems. > Will do so. Please note that I have deleted the *empty* patchsets that had got created due to the tool bug. The patchset that was LGTMed is still available here for integration.
Sign in to reply to this message.
Any more comments about this CL? I will wait for 1 day before submitting.
Sign in to reply to this message.
LGTM make sure he's in the CLA
Sign in to reply to this message.
On 2012/09/24 09:40:21, r wrote: > LGTM > make sure he's in the CLA I have signed(submitted) the Individual CLA.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=b6d8227d99ae *** codereview: use subprocess.call() instead of os.spawnvp() for portability Fixes issue 4121. R=golang-dev, minux.ma, dave, r CC=golang-dev http://codereview.appspot.com/6555049 Committer: Shenghou Ma <minux.ma@gmail.com>
Sign in to reply to this message.
|