Initial cut at generating the AnalysisServer.java interface. Currently this generates a commented o…

Initial cut at generating the AnalysisServer.java interface. Currently this generates a commented out interface as the java server is not up to date with the spec.

This work iterates on the work that Paul started.

R=brianwilkerson@google.com, paulberry@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39155

[scheglov, 2014-08-12 17:34:29]

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java
(right):

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:31:
* Add the given listener to the list of listeners that will receive notification
when new
Extra spaces before "*".

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:54:
// public void createContext(String contextRoot, CreateContextConsumer
consumer);
I think we should add domain name.
It is "createDebugContext" currently.
"debug_createContext" ideally for me.

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:76:
* the search.
This wrapped @param line should be indented.

[jwren, 2014-08-12 17:43:48]

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java
(right):

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:31:
* Add the given listener to the list of listeners that will receive notification
when new
Fixed.

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:54:
// public void createContext(String contextRoot, CreateContextConsumer
consumer);
I considered this, I was trying to match the API that Brian originally came up
with, but agree that "createContext" doesn't give much context.

Any objections...?

On 2014/08/12 17:34:29, scheglov wrote:
> I think we should add domain name.
> It is "createDebugContext" currently.
> "debug_createContext" ideally for me.

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:76:
* the search.
On 2014/08/12 17:34:29, scheglov wrote:
> This wrapped @param line should be indented.

I saw this too, we could fix it in the generator, but I'm not convinced it is
worth it to spend the time.

[jwren, 2014-08-12 17:48:59]

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java
(right):

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:54:
// public void createContext(String contextRoot, CreateContextConsumer
consumer);
Some advantages: it will match the order of the spec, all the domains will be
organized, the non-spec defined methods (start and addAnalysisServerListener)
are easy to spot.

On 2014/08/12 17:43:47, jwren wrote:
> I considered this, I was trying to match the API that Brian originally came up
> with, but agree that "createContext" doesn't give much context.
> 
> Any objections...?
> 
> On 2014/08/12 17:34:29, scheglov wrote:
> > I think we should add domain name.
> > It is "createDebugContext" currently.
> > "debug_createContext" ideally for me.
>

[scheglov, 2014-08-12 17:52:35]

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java
(right):

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:54:
// public void createContext(String contextRoot, CreateContextConsumer
consumer);
On 2014/08/12 17:48:58, jwren wrote:
> Some advantages: it will match the order of the spec, all the domains will be
> organized, the non-spec defined methods (start and addAnalysisServerListener)
> are easy to spot.
> 
> On 2014/08/12 17:43:47, jwren wrote:
> > I considered this, I was trying to match the API that Brian originally came
up
> > with, but agree that "createContext" doesn't give much context.
> > 
> > Any objections...?
> > 
> > On 2014/08/12 17:34:29, scheglov wrote:
> > > I think we should add domain name.
> > > It is "createDebugContext" currently.
> > > "debug_createContext" ideally for me.
> > 
> 

  Alternatively we could have an explicit Domain class with methods.
  And Server will have final fields like "analysis", "edit", etc.

[Brian Wilkerson, 2014-08-12 17:58:02]

LGTM

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java
(right):

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:54:
// public void createContext(String contextRoot, CreateContextConsumer
consumer);
I'd choose the naming convention rather than explicit Domain classes so that
it's easier to implement, but it's your call.

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_analysis_server.dart (right):

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_analysis_server.dart:8: library
CodegenAnalysisServer;
Perhaps "server.generator.java"? (They're usually dotted rather than
camel-case.)

[Paul Berry, 2014-08-12 18:04:02]

It looks like you moved around some writeln()'s that might affect Dart code
generation (but I'm not certain).  Did you rerun the generate_files script to
make sure nothing else changed?

Other than that, the only comment I feel strongly about is the break vs.
continue issue.  The rest are your call.

lgtm.

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java
(right):

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:54:
// public void createContext(String contextRoot, CreateContextConsumer
consumer);
On 2014/08/12 17:48:58, jwren wrote:
> Some advantages: it will match the order of the spec, all the domains will be
> organized, the non-spec defined methods (start and addAnalysisServerListener)
> are easy to spot.
> 
> On 2014/08/12 17:43:47, jwren wrote:
> > I considered this, I was trying to match the API that Brian originally came
up
> > with, but agree that "createContext" doesn't give much context.
> > 
> > Any objections...?
> > 
> > On 2014/08/12 17:34:29, scheglov wrote:
> > > I think we should add domain name.
> > > It is "createDebugContext" currently.
> > > "debug_createContext" ideally for me.
> > 
> 

FYI, in the Dart generated code I used camelJoin() for things like this (so the
name would be "debugCreateContext")

https://codereview.chromium.org/463163002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/AnalysisServer2.java:76:
* the search.
On 2014/08/12 17:43:47, jwren wrote:
> On 2014/08/12 17:34:29, scheglov wrote:
> > This wrapped @param line should be indented.
> 
> I saw this too, we could fix it in the generator, but I'm not convinced it is
> worth it to spend the time.

I have an idea of how to fix this; I'll look into it after the CL lands.

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_analysis_server.dart (right):

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_analysis_server.dart:56: switch
(request.longMethod) {
Maybe drop this do-nothing switch statement?

(I think it was probably left over from hacks in the code you inherited from me)

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_analysis_server.dart:67: if
(request.params != null) {
Rather than do all the bookkeeping about commas, consider something like this:

List<String> arguments = [];
if (request.params != null) {
  for (TypeObjectField field in request.params.fields) {
    arguments.add('${javaType(field.type)} ${javaName(field.name)}');
  }
}
if (request.result != null) {
  arguments.add('${consumerName(request)} consumer');
}
write(arguments.join(', '));

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/to_html.dart (right):

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/to_html.dart:313: break;
Should be "continue;"

[jwren, 2014-08-12 18:23:08]

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_analysis_server.dart (right):

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_analysis_server.dart:8: library
CodegenAnalysisServer;
On 2014/08/12 17:58:02, Brian Wilkerson wrote:
> Perhaps "server.generator.java"? (They're usually dotted rather than
> camel-case.)

Done.

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_analysis_server.dart:56: switch
(request.longMethod) {
On 2014/08/12 18:04:01, Paul Berry wrote:
> Maybe drop this do-nothing switch statement?
> 
> (I think it was probably left over from hacks in the code you inherited from
me)

Done.

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_analysis_server.dart:67: if
(request.params != null) {
I looked for join, but didn't find it... this is much cleaner.

On 2014/08/12 18:04:02, Paul Berry wrote:
> Rather than do all the bookkeeping about commas, consider something like this:
> 
> List<String> arguments = [];
> if (request.params != null) {
>   for (TypeObjectField field in request.params.fields) {
>     arguments.add('${javaType(field.type)} ${javaName(field.name)}');
>   }
> }
> if (request.result != null) {
>   arguments.add('${consumerName(request)} consumer');
> }
> write(arguments.join(', '));

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/to_html.dart (right):

https://codereview.chromium.org/463163002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/to_html.dart:313: break;
On 2014/08/12 18:04:02, Paul Berry wrote:
> Should be "continue;"

Done.

[jwren, 2014-08-12 18:34:05]

Committed patchset #5 manually as 39155 (presubmit successful).