Refactoring PHP generation to use the MVVM pattern #317

garrettjonesgoogle · 2016-07-19T23:08:11Z

This was prototyped in #268
for both Java and PHP; the present PR only includes PHP and shared
stuff.

The MVVM pattern uses the following steps:

The Model is converted to a ViewModel, which is a set of classes
containing the data to be used when rendering the templates.
The snippet engine renders the output using only data from the
ViewModel structure.

Other changes with the MVVM model:

There is no context object when generating under the MVVM model. All
data must be prepared beforehand.
No casing changes are done in templates; all identifiers must be
composed and cased correctly inside the view model.

There are a number of new utility classes created:

Name: represents an identifier name which is casing-aware.
TypeName: Represents a simple or complex type and keeps track of the
aliases for the contributing types.
TypeTable: manages the imports for a set of fully-qualified type
names.
SurfaceNamer: provides language-specific names or other strings.

There is also a change unrelated to MVVM:

Using TreeSet for auth scopes to give a consistent ordering

This was prototyped in googleapis#268 for both Java and PHP; the present PR only includes PHP and shared stuff. The MVVM pattern uses the following steps: 1. The Model is converted to a ViewModel, which is a set of classes containing the data to be used when rendering the templates. 2. The snippet engine renders the output using only data from the ViewModel structure. Other changes with the MVVM model: * There is no context object when generating under the MVVM model. All data must be prepared beforehand. * No casing changes are done in templates; all identifiers must be composed and cased correctly inside the view model. There are a number of new utility classes created: * Name: represents an identifier name which is casing-aware. * TypeName: Represents a simple or complex type and keeps track of the aliases for the contributing types. * TypeTable: manages the imports for a set of fully-qualified type names. * SurfaceNamer: provides language-specific names or other strings. There is also a change unrelated to MVVM: * Using TreeSet for auth scopes to give a consistent ordering

src/main/java/com/google/api/codegen/transformer/ApiMethodTransformer.java

+ * ApiMethodTransformer generates view objects from method definitions.
+ */
+public class ApiMethodTransformer {
+  private InitCodeTransformer initCodeTransformer;


garrettjonesgoogle · 2016-07-22T21:45:27Z

addressed comments by either making changes or responding to them - PTAL

src/main/java/com/google/api/codegen/viewmodel/ParamView.java

+ */
+package com.google.api.codegen.viewmodel;
+
+public interface ParamView {}


src/main/java/com/google/api/codegen/util/Name.java

+   * @throws IllegalArgumentException if any of the strings contain any characters that are not
+   * lower case or underscores.
+   */
+  public static Name from(String... pieces) {


src/main/java/com/google/api/codegen/gapic/ViewModelGapicProvider.java

+      }
+      Doc doc = snippetSetRunner.generate(surfaceDoc);
+      if (doc == null) {
+        continue;


src/main/java/com/google/api/codegen/php/PhpSnippetSetRunner.java

@@ -48,15 +49,15 @@ public GeneratedResult generate(
            ImmutableMap.<String, Object>of("context", context));

    String outputFilename = snippets.generateFilename(element).prettyPrint();
-    PhpContextCommon phpContextCommon = new PhpContextCommon();
+    PhpTypeTable phpTypeTable = new PhpTypeTable();

    // TODO don't depend on a cast here
    PhpContext phpContext = (PhpContext) context;


bjwatson · 2016-07-27T21:30:21Z

Are the context methods like lowerUnderscoreToLowerCamel() effectively dead code now that can be removed, thanks to the new namer classes?

src/test/java/com/google/api/codegen/testdata/php_main_library.baseline

@@ -1207,7 +1213,7 @@ class LibraryServiceApi
     * try {
     *     $libraryServiceApi = new LibraryServiceApi();
     *     $formattedName = LibraryServiceApi::formatBookName("[SHELF]", "[BOOK]");
-     *     $indexName = "";
+     *     $indexName = "default index";


bjwatson · 2016-07-28T00:17:40Z

A general comment is that the src/main/java/com/google/api/codegen/viewmodel code could use a lot more documentation. These POJOs are central to how MVVM works, and explanation is needed for how they're structured.

This could wait until the Java PR, since I understand that there might be some active shuffling around of these fields right now.

bjwatson · 2016-07-28T00:41:13Z

There are a surprising number of public methods in src/main/java/com/google/api/codegen/transformer. I would expect that only the two methods of ModelToViewTransformer need to be public, since that is the interface with the rest of the toolkit code. Everything else should ideally be protected or package private.

garrettjonesgoogle · 2016-07-28T01:12:52Z

A general comment is that the src/main/java/com/google/api/codegen/viewmodel code could use a lot more documentation. These POJOs are central to how MVVM works, and explanation is needed for how they're structured.

This could wait until the Java PR, since I understand that there might be some active shuffling around of these fields right now.

These fields aren't likely to shuffle. Since the fields are simple POJOs, for the most part, any documentation would be a conversion from the camel case of the identifier into noun phrase form. Additional details would probably only talk about where they are used, which would only refer to other code, which isn't much of a value-add. My hope is that the naming of the fields is transparent such that they don't need documentation.

There are a surprising number of public methods in src/main/java/com/google/api/codegen/transformer. I would expect that only the two methods of ModelToViewTransformer need to be public, since that is the interface with the rest of the toolkit code. Everything else should ideally be protected or package private.

I will convert a few more methods to private that are only needed within their own class.

In general, as I argued in the previous PR, I don't think making things package private buys us very much in the context of toolkit where no one is depending on us. I would prefer to do a more thorough pass of access levels when we want to convert into a library for others to consume & extend.

garrettjonesgoogle · 2016-07-28T01:22:37Z

Made some updates, PTAL

bjwatson · 2016-07-28T18:06:00Z

In general, as I argued in the previous PR, I don't think making things package private buys us very much in the context of toolkit where no one is depending on us. I would prefer to do a more thorough pass of access levels when we want to convert into a library for others to consume & extend.

I don't think I ever responded to that. I think that in any large project, we should use the lowest level of access possible for just about everything (with the possible exception of constants). It's not just about us vs. them; it's also about what we can hold in our head at any given time, and it's helpful to see that the compiler enforces a limited interface between a model and the rest of the application.

We don't need to wait for a thorough audit to start moving in that direction now. Code cleanups, documentation, unit testing, etc. can and should happen incrementally as we do our other features.

src/test/java/com/google/api/codegen/testdata/php_main_library.baseline

@@ -230,6 +228,7 @@ class LibraryServiceApi
        return self::$archivedBookNameTemplate;
    }

+


bjwatson · 2016-07-28T18:28:23Z

These fields aren't likely to shuffle. Since the fields are simple POJOs, for the most part, any documentation would be a conversion from the camel case of the identifier into noun phrase form. Additional details would probably only talk about where they are used, which would only refer to other code, which isn't much of a value-add. My hope is that the naming of the fields is transparent such that they don't need documentation.

I wouldn't count on that. In some cases it's obvious, but in many cases it isn't. You're very familiar with this code in a way that others are not. Additional details could take the form of code examples in one or two languages, to show what it represents in concrete terms. This does provide value-add that an IDE cannot easily do for you, because it grounds the abstract concepts in the viewmodel to make them easily understandable for other developers who don't yet have the mental model that you do.

Like I said, I won't hold up this PR for documenting these classes, but I would like to see this improved in the near future while it's still fresh in your mind. A refactoring is a good opportunity to improve our game in terms of things like documentation and unit testing.

garrettjonesgoogle · 2016-07-28T18:52:17Z

In general, as I argued in the previous PR, I don't think making things package private buys us very much in the context of toolkit where no one is depending on us. I would prefer to do a more thorough pass of access levels when we want to convert into a library for others to consume & extend.

I don't think I ever responded to that. I think that in any large project, we should use the lowest level of access possible for just about everything (with the possible exception of constants). It's not just about us vs. them; it's also about what we can hold in our head at any given time, and it's helpful to see that the compiler enforces a limited interface between a model and the rest of the application.

We don't need to wait for a thorough audit to start moving in that direction now. Code cleanups, documentation, unit testing, etc. can and should happen incrementally as we do our other features.

I understand the idea of using the most restrictive visibility. The problem here is that the MVVM feature cuts across quite a few packages, all of which need to interact. There are components that I would ideally like to hide, but which need to be public because the need to be accessed in another package of MVVM. For example, the language specific transformers (which are in a different package as per the architectural restructuring suggested by you & Shin) need to access at least one public method in each of the general transformers. Thus, every transformer needs to be public.

A good language feature would be to allow multiple packages to be part of a single visibility scope, but alas, we have no such thing. (C# seems to have an "internal" visibility level which limits visibility to an "assembly", which is close to what I want).

bjwatson · 2016-07-28T19:23:41Z

A good language feature would be to allow multiple packages to be part of a single visibility scope, but alas, we have no such thing. (C# seems to have an "internal" visibility level which limits visibility to an "assembly", which is close to what I want).

Okay, I think we're on the same page with this. Java unfortunately has no true concept of sub-packages, unlike Scala or C#. I think there might be ways to whittle down the visibility given that everything in the language-specific packages are subclasses of the common transformer types, but we can defer that argument until after this PR.

garrettjonesgoogle · 2016-07-28T19:50:39Z

Okay, I think we're on the same page with this. Java unfortunately has no true concept of sub-packages, unlike Scala or C#. I think there might be ways to whittle down the visibility given that everything in the language-specific packages are subclasses of the common transformer types, but we can defer that argument until after this PR.

Cool.

I do want to note that for the key interaction I mentioned, between PhpGapicSurfaceTransformer and *Transformer in the general package, PhpGapicSurfaceTransformer has no structural relation to the *Transformer types; it only implements the interface ModelToViewTransformer. I have no idea how to give special access to PhpGapicSurfaceTransformer to make calls to the *Transformer types without contortions that would make the code more opaque and harder to maintain. We can discuss after the PR though.

bjwatson · 2016-07-28T20:04:00Z

LGTM.

Let's have a 1:1 next week to discuss my remaining concerns around documentation and access modifiers. I'll put something on your calendar.

garrettjonesgoogle assigned bjwatson and shinfan Jul 19, 2016

This was referenced Jul 19, 2016

DO NOT MERGE: Java MVVM garrettjonesgoogle/toolkit#1

Closed

DO NOT MERGE: MVVM prototyping #268

Closed

garrettjonesgoogle assigned michaelbausor Jul 19, 2016

shinfan reviewed Jul 19, 2016
View reviewed changes

garrettjonesgoogle added 2 commits July 22, 2016 13:39

InitCodeGenerator: using Name for suggestedName

046b5c3

breaking apart long method

a0fc1d8

michaelbausor reviewed Jul 26, 2016
View reviewed changes

src/main/java/com/google/api/codegen/viewmodel/ParamView.java

*/

package com.google.api.codegen.viewmodel;

public interface ParamView {}

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

PR feedback

01e30eb

michaelbausor reviewed Jul 26, 2016
View reviewed changes

garrettjonesgoogle added 2 commits July 26, 2016 13:49

Renaming ViewNamer to NameFormatterMixin

6196a6d

Decomposing type naming functionality further

0bc4f9d

bjwatson reviewed Jul 27, 2016
View reviewed changes

Addressing PR comments

376ae35

bjwatson reviewed Jul 28, 2016
View reviewed changes

src/test/java/com/google/api/codegen/testdata/php_main_library.baseline

@@ -230,6 +228,7 @@ class LibraryServiceApi

return self::$archivedBookNameTemplate;

}

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

blank line fixes

60005b2

Renaming to NameFormatterDelegator

68c5b2e

garrettjonesgoogle merged commit d9cea89 into googleapis:master Jul 28, 2016

bjwatson mentioned this pull request Aug 2, 2016

Improve Go's Import Name Handling #339

Closed

		@@ -230,6 +228,7 @@ class LibraryServiceApi
		return self::$archivedBookNameTemplate;
		}

Refactoring PHP generation to use the MVVM pattern #317

Refactoring PHP generation to use the MVVM pattern #317

Conversation

garrettjonesgoogle commented Jul 19, 2016

This comment was marked as spam.

This comment was marked as spam.

garrettjonesgoogle commented Jul 22, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

bjwatson commented Jul 27, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

bjwatson commented Jul 28, 2016

bjwatson commented Jul 28, 2016

garrettjonesgoogle commented Jul 28, 2016

garrettjonesgoogle commented Jul 28, 2016

bjwatson commented Jul 28, 2016

This comment was marked as spam.

This comment was marked as spam.

bjwatson commented Jul 28, 2016

garrettjonesgoogle commented Jul 28, 2016

bjwatson commented Jul 28, 2016

garrettjonesgoogle commented Jul 28, 2016

bjwatson commented Jul 28, 2016