Given that this is a tool that needs to be compiled, it'd be nice to make this its own top-level module in /trunk and add a Makefile for compiling it.

Also, is there any reason we need a wrapper? The script can't just run python post-review or maybe a compiled post-review.exe?

1. JSlominski 3 months, 3 weeks ago (July 25th, 2008, 4:13 p.m.)

   The reason I out this in a wrapper is because post-review will take the client and server that is set in the P4 ENV variables.  By setting up the environment properly before running python post-review, users can submit reviews from multiple client specs without any hassle.  I’m not sure how other places use clients but we have a separate client for each branch of software we release from.  I could set up a compiled version of this but then I would need to make the command line arguments more robust.  If you think that would be a better approach I can start working on that on Monday. 

2. Christian Hammond 3 months, 3 weeks ago (July 25th, 2008, 4:47 p.m.)

   Maybe what we should do is have some Perforce-specific arguments in post-review for setting these variables instead of getting them from the environment.

3. JSlominski 3 months, 3 weeks ago (July 28th, 2008, 11:40 a.m.)

   Should I abandon this and make post-review.py more robust?

4. Christian Hammond 2 months, 3 weeks ago (August 27th, 2008, 2:48 a.m.)

   Sorry, missed your question before.
   
   Yeah, it might be worth making post-review more robust so that it can be used in other ways. Having these Perforce-specific arguments could be useful in all kinds of scripts.

/trunk/reviewboard/contrib/tools/P4post-review.cpp (Diff revision 1)
		81	sprintf(command,"set P4CLIENT=%s", argv[2]); //set Perforce ENV

You can use setenv for this.

1. JSlominski 3 months, 3 weeks ago (July 25th, 2008, 4:13 p.m.)

   I know setenv() works on Unix platforms but windows didn't like it.  I took the easy route and did this.  Ill do some more digging on Monday to see if I can find the proper function. 

2. JSlominski 3 months, 3 weeks ago (July 28th, 2008, 11:40 a.m.)

   Fixed this for the windows platform

/trunk/reviewboard/contrib/tools/post-review (Diff revision 1)
		1561

This line can go.

1. JSlominski 3 months, 3 weeks ago (July 25th, 2008, 4:04 p.m.)

   Not really sure how this got here

/trunk/reviewboard/contrib/tools/post-review (Diff revision 3)
		807	if options.p4Client != None:

You don't need "!= None" here.

/trunk/reviewboard/contrib/tools/post-review (Diff revision 3)
		1497	if type(tool) == PerforceClient:

You should be using isinstance() here.

/trunk/reviewboard/contrib/tools/post-review (Diff revision 3)
		1498	parser.add_option("--P4Client",

I'd prefer --p4-client and --p4-port. The variable names should be p4_client and p4_port too.

/trunk/reviewboard/contrib/tools/post-review (Diff revision 3)
		1500	help="the Perforce client spec name that the CL is in")

I'd rather we not abbreviate "CL". This is more generic than changelists, so let's just say something like "the Perforce client name" and "the Perforce server address" or something.

Thanks, committed as r1550.

I don't use Windows and therefore don't use P4win, so I can't test this, but what happens if you need to log in? Does P4win provide any sort of a terminal UI where you can input this information, and does it show the resulting URL?

We may want to modify this to use -o (for opening a web browser) after this is posted.