[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[mdempsky]

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[skybrian]

On 2012/10/24 21:24:02, mdempsky wrote:

Alternatively, take a look at Patch Set #1, which simply added an

extra

parameter to exec() to pass in a custom JavaCompiler instance.  Would

that patch

(updated to make the exec() methods public) be preferable?


Yeah, seems simpler.

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[mdempsky]

Alternatively, take a look at Patch Set #1, which simply added an extra
parameter to exec() to pass in a custom JavaCompiler instance.  Would
that patch (updated to make the exec() methods public) be preferable?

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[mdempsky]

http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
File
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
(right):

http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java#newcode216
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java:216:
protected JavaCompiler getJavaCompiler() {
For no particular reason, I was trying to defer creating the
JavaCompiler until it's actually needed.  If this code was already using
JSR330, I'd add a constructor that takes a Provider and
handle it that way.  Since it's not and I don't expect to see too many
custom ValidationTool subclasses, just adding an override method seemed
the least intrusive solution.

http://gwt-code-reviews.appspot.com/1859803/diff/6001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
File
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
(right):

http://gwt-code-reviews.appspot.com/1859803/diff/6001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java#newcode212
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java:212:
public void run(String[] args) throws IOException {
On 2012/10/24 20:56:48, tbroyer wrote:

Er, why this method?


Mostly so that I can just write:

class FancyValidationTool extends ValidationTool {
  public static void main(String[] args) throws IOException {
new FancyValidationTool().run(args);
  }

  protected JavaCompiler getJavaCompiler() { ... }
}

I could reproduce the System.exit() logic in FancyValidationTool too,
but this way just seemed simpler (and ValidationTool already does things
like write to System.err directly).  I'm fine with making main()
responsible for calling System.exit() if you find this way distasteful.

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[t . broyer]

http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
File
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
(right):

http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java#newcode216
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java:216:
protected JavaCompiler getJavaCompiler() {
On 2012/10/24 20:52:30, skybrian wrote:

Not a big deal, but I usually prefer to introduce a field and

constructor arg

for configuration, to avoid starting a class hierarchy.


+1

http://gwt-code-reviews.appspot.com/1859803/diff/6001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
File
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
(right):

http://gwt-code-reviews.appspot.com/1859803/diff/6001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java#newcode212
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java:212:
public void run(String[] args) throws IOException {
Er, why this method? Couldn't the System.exit be in the main()?

  System.exit(new ValidationTool().exec(args) ? 0 : -1);

or

  boolean successful = new ValidationTool().exec(args);
  System.exit(successful ? 0 : -1);

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[skybrian]

http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
File
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
(right):

http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java#newcode216
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java:216:
protected JavaCompiler getJavaCompiler() {
Not a big deal, but I usually prefer to introduce a field and
constructor arg for configuration, to avoid starting a class hierarchy.

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[mdempsky]

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[mdempsky]

http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
File
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
(right):

http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java#newcode224
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java:224:
if (args.length < 2) {
On 2012/10/24 20:37:40, jtamplin wrote:

Should this be public so other tools have a way to run it in-process

without

having to worry about the System.exit call?


Sounds reasonable to me.  I'll upload a revised patch set in a moment.

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[jat]

LGTM


http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
File
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java
(right):

http://gwt-code-reviews.appspot.com/1859803/diff/2001/user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java#newcode224
user/src/com/google/web/bindery/requestfactory/apt/ValidationTool.java:224:
if (args.length < 2) {
Should this be public so other tools have a way to run it in-process
without having to worry about the System.exit call?

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Split ValidationTool.exec() into two methods so alternative (issue1859803)

[mdempsky]

http://gwt-code-reviews.appspot.com/1859803/

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors