Refactored WTP new project code to easily extend to include flex support. by nbashirbello · Pull Request #1497 · GoogleCloudPlatform/google-cloud-eclipse

nbashirbello · 2017-02-28T15:43:06Z

No description provided.

the CT4E dropdown

Property tester works

support.

nbashirbello · 2017-02-28T15:43:15Z

PR not ready for review

a different PR

nbashirbello · 2017-02-28T16:24:02Z

PR ready for review

elharo · 2017-02-28T16:22:32Z

 import org.junit.Before;
 import org.junit.Test;

 public class StandardProjectWizardTest {


should probably rename this too

elharo · 2017-02-28T16:23:21Z


 /**
- * Collects all data needed to create and configure an App Engine Standard Project.
+ * Collects all data needed to create and configure an App Engine Project.


App Engine Eclipse project.

elharo · 2017-02-28T16:23:38Z

+ * Collects all data needed to create and configure an App Engine Project.
 */
-class AppEngineStandardProjectConfig {
+public class AppEngineProjectConfig {


I start to get nervous when methods get more public. This is now exposing mutable internal state publicly.

elharo · 2017-02-28T16:25:20Z

    setNeedsProgressMonitor(true);
  }

+  public abstract AppEngineWizardPage getWizardPage();


It looks like this may not be a pure getter method if it's not returning the value of the page field. If so, please rename. Maybe createWizardPage()?

elharo · 2017-02-28T16:28:27Z

+  private ILibraryClasspathContainerResolverService resolverService;
+
+  public AppEngineStandardProjectWizard(){
+    super();


delete this line since it's automatic

elharo · 2017-02-28T16:28:53Z

+  }
+
+  @Override
+  public AppEngineWizardPage getWizardPage() {


yes, definitely rename this method since getters aren't creators.

chanseokoh · 2017-02-28T16:49:24Z


    try (InputStream contentXml = new FileInputStream(
-        "../com.google.cloud.tools.eclipse.appengine.newproject/helpContexts.xml")) {
+        "../com.google.cloud.tools.eclipse.appengine.newproject/standardHelpContexts.xml")) {


It's kind of convention to use the name helpContexts.xml. We also test if the file is included in build.properties: https://github.com/GoogleCloudPlatform/google-cloud-eclipse/blob/master/plugins/com.google.cloud.tools.eclipse.test.util/src/com/google/cloud/tools/eclipse/test/util/BasePluginXmlTest.java#L112

I think you can keep the name helpContexts.xml, and put both standard and flex help contexts into this file.

chanseokoh · 2017-02-28T16:53:25Z

               plugin.properties,\
               plugin.xml,\
-               helpContexts.xml
+               standardHelpContexts.xml


chanseokoh · 2017-02-28T16:53:34Z

        point="org.eclipse.help.contexts">
     <contexts
-           file="helpContexts.xml">
+           file="standardHelpContexts.xml">


chanseokoh · 2017-02-28T16:56:23Z

+
+  public abstract void sendAnalyticsPing();
+
+  public abstract IStatus validateDependencies(boolean fork, boolean cancelable);


The two arguments are always true, so I don't see a great value to accept them.

chanseokoh · 2017-02-28T16:58:28Z

+  }
+
+  @Override
+  public void sendAnalyticsPing(Composite parent) {


Please rename parent to shell, and let the caller pass parent.getShell().

chanseokoh · 2017-02-28T17:01:58Z

+  @Override
+  public void setHelp(Composite container) {
+    PlatformUI.getWorkbench().getHelpSystem().setHelp(container,
+        "com.google.cloud.tools.eclipse.appengine.newproject.NewStandardProjectContext"); //$NON-NLS-1$


You'll have to duplicate PlatformUI.getWorkbench().getHelpSystem().setHelp(container, ...) in flex too.

Somewhat in a similar vein as https://github.com/GoogleCloudPlatform/google-cloud-eclipse/pull/1497/files#r103506394, I wonder if it's better to accept a help context ID (the literal string here) through a constructor (accepting null) and just have this code at the base class instead of having this extra setHelp() method. It's your preference though.

Same as the earlier comment.

chanseokoh · 2017-02-28T17:08:42Z

+
+  public abstract String getCreateNewProjectOperationLabel();
+
+  public abstract IFile createProjectFiles(IProject newProject, AppEngineProjectConfig config,


Please add a javadoc saying that this method should return the most important file that the IDE will auto-open.

chanseokoh · 2017-02-28T17:09:22Z


+  public abstract void addAppEngineFacet(IProject newProject, IProgressMonitor monitor) throws CoreException;
+
+  public abstract String getCreateNewProjectOperationLabel();


It will be helpful to add a Javadoc about where the label will be used.

I'm not experienced in design patterns, but I think you don't really have to have an abstract method if what you want to get is just a literal value? That is, you can have a field for this label here and let subclasses pass the value through a constructor. Thoughts?

getDescription()?

Both approaches would work in this case. It's not uncommon for abstract methods to provide labels and descriptions.

chanseokoh · 2017-02-28T17:17:01Z

+  public AppEngineStandardWizardPage() {
+    super();
+    setTitle(Messages.getString("app.engine.standard.project")); //$NON-NLS-1$
+    setDescription(Messages.getString("create.app.engine.standard.project")); //$NON-NLS-1$


What do you think about passing a title and a description through arguments. (No strong opinion here.)

chanseokoh · 2017-02-28T17:22:55Z

+
+  public AppEngineStandardProjectWizard(){
+    super();
+    setWindowTitle(Messages.getString("new.app.engine.standard.project"));


You forgot to call this in the flex counterpart. How about accepting the title as an argument?

chanseokoh · 2017-02-28T19:57:37Z

    Composite container = (Composite) getControl();
-    PlatformUI.getWorkbench().getHelpSystem().setHelp(container,
-        "com.google.cloud.tools.eclipse.appengine.newproject.NewProjectContext"); //$NON-NLS-1$
+    setHelp(container.getShell());


I think this should be container.

briandealwis · 2017-02-28T19:11:32Z

    setNeedsProgressMonitor(true);
  }

+  public abstract AppEngineWizardPage createWizardPage();


Which wizard page? Is this the main page?

Yes, there is only one page for creating the wtp project.

briandealwis · 2017-02-28T19:15:07Z

-import org.eclipse.wst.common.project.facet.core.ProjectFacetsManager;

 /**
 * Utility to make a new Eclipse project with the App Engine Standard facets in the workspace.


Missed a "standard"

briandealwis · 2017-02-28T19:15:53Z


+  public abstract void addAppEngineFacet(IProject newProject, IProgressMonitor monitor) throws CoreException;
+
+  public abstract String getCreateNewProjectOperationLabel();


getDescription()?

briandealwis · 2017-02-28T20:37:02Z

  private String serviceName;

-  File getCloudSdkLocation() {
+  public File getCloudSdkLocation() {


Aside: seems odd for the Cloud SDK location to be here.

briandealwis · 2017-02-28T20:40:53Z

+
+  public abstract IStatus validateDependencies(boolean fork, boolean cancelable);
+
+  public abstract CreateAppEngineWtpProject getAppEngineProjectCreationOperation(


This seems odd. I wonder if we wouldn't be better to have an abstract createProject() operation like what you've done for validateDependencies().

Chatted offline. Will leave as is since all the flex operation are workspace operations

briandealwis · 2017-02-28T20:43:01Z

+   * Returns a user visible name for the resource operation that generates the files
+   * for the App Engine WTP project.
+   */
+  public abstract String getCreateNewProjectOperationLabel();


Not a big fan of this name. getDescription()?

briandealwis · 2017-02-28T20:45:47Z

+import org.eclipse.swt.widgets.Composite;
+import org.eclipse.swt.widgets.Shell;
+
+public class AppEngineFlexWizardPage extends AppEngineWizardPage {


I'm curious to see how this will be different from the AppEngineStandardWizardPage. If it's just the ping-type and help id, then we could make those configurable items passed in during creation?

This will be expanded in a subsequent PR.

briandealwis · 2017-02-28T20:46:54Z

+    AnalyticsPingManager.getInstance().sendPing(
+        AnalyticsEvents.APP_ENGINE_NEW_PROJECT_WIZARD_COMPLETE,
+        AnalyticsEvents.APP_ENGINE_NEW_PROJECT_WIZARD_TYPE,
+        AnalyticsEvents.APP_ENGINE_NEW_PROJECT_WIZARD_TYPE_NATIVE);


Do we need to add something here to differentiate between std and flex?

This would be done in a subsequent PR.

briandealwis · 2017-03-01T19:12:25Z

-

-</plugin>
+


I don't think we need two blank lines?

briandealwis · 2017-03-01T19:29:29Z

      sdk.validateCloudSdk();
      sdk.validateAppEngineJavaComponents();
-      page = new AppEngineStandardWizardPage();
+      page = createWizardPage();


My 2¢: since it's important to have the Cloud SDK validation test here, I'd prefer following the addPages() pattern here and delegate the add-page to an abstract method like addAppEnginePages(). This gives AppEngine Std and Flex the flexibility to independently add multiple pages should they need to (e.g., Flex may need to select a runtime?) without having to refactor this or figure out how to shoehorn the functionality into a single page.

codecov-io · 2017-03-02T01:37:33Z

Codecov Report

Merging #1497 into master will increase coverage by 0.87%.
The diff coverage is 72.15%.

@@             Coverage Diff              @@
##             master    #1497      +/-   ##
============================================
+ Coverage     69.91%   70.79%   +0.87%     
- Complexity     1237     1349     +112     
============================================
  Files           219      230      +11     
  Lines          8796     9239     +443     
  Branches        743      790      +47     
============================================
+ Hits           6150     6541     +391     
- Misses         2331     2380      +49     
- Partials        315      318       +3

Impacted Files	Coverage Δ	Complexity Δ
...ls/eclipse/appengine/newproject/CodeTemplates.java	`88% <ø> (ø)`	`9 <0> (ø)`	❌
...newproject/flex/CreateAppEngineFlexWtpProject.java	`0% <0%> (ø)`	`0 <0> (?)`
...ne/newproject/flex/AppEngineFlexProjectWizard.java	`0% <0%> (ø)`	`0 <0> (?)`
...ngine/newproject/flex/AppEngineFlexWizardPage.java	`0% <0%> (ø)`	`0 <0> (?)`
...ppengine/newproject/CreateAppEngineWtpProject.java	`97.36% <100%> (ø)`	`5 <0> (?)`
...wproject/standard/AppEngineStandardWizardPage.java	`100% <100%> (ø)`	`3 <3> (?)`
...ct/standard/CreateAppEngineStandardWtpProject.java	`100% <100%> (ø)`	`4 <4> (?)`
...e/appengine/newproject/AppEngineProjectWizard.java	`64.58% <100%> (ø)`	`4 <2> (?)`
...ipse/appengine/newproject/AppEngineWizardPage.java	`85.93% <100%> (ø)`	`12 <0> (?)`
...e/appengine/newproject/AppEngineProjectConfig.java	`100% <100%> (ø)`	`13 <1> (?)`
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aa34d0...ec3d553. Read the comment docs.

nbashirbello added 6 commits February 25, 2017 15:24

Added menu items for creating a new flex project under File.. and under

301a99f

the CT4E dropdown

Merge branch 'master' into newFlexProjectWizard

0f24793

Merged master and moved FlexFacetPropertyTester into new flex bundle.

f973b65

Property tester works

Moved flex new project declaration into new flex bundle

9c46bf0

Added help context for Flex

ba8c862

Refactored WTP new project code to easily extend to include flex

04b5d37

support.

googlebot added the cla: yes label Feb 28, 2017

nbashirbello added 5 commits February 28, 2017 11:00

Removed code for viewing flex new project wizard. Will move this code to

ec60e6b

a different PR

Removed FlexFacetPropertyTester.

fb15898

Clean up

d488d8d

Removed some flex ui stuff

5f96960

Reverted c.g.c.t.e.appengine.flex/plugin.xml

dbe6cf1

nbashirbello requested a review from briandealwis February 28, 2017 16:24

elharo reviewed Feb 28, 2017

View reviewed changes

Fixed path to AppEngineStandardProjectWizard

f9bdd6b

chanseokoh reviewed Feb 28, 2017

View reviewed changes

nbashirbello added 3 commits February 28, 2017 12:34

Minor changes per PR comments

56a8dbd

Renamed standardHelpContexts.xml back to helpContents.xml

1d42ea3

Minor changes per PR comments

ed807f2

chanseokoh reviewed Feb 28, 2017

View reviewed changes

Passed container to setHelp in AppEngineWizardPage#createControl

d7f3246

briandealwis reviewed Feb 28, 2017

View reviewed changes

Minor changes per PR comments.

81a3a83

briandealwis approved these changes Mar 1, 2017

View reviewed changes

Remove extra blank line

ec3d553

nbashirbello merged commit 8aae460 into master Mar 2, 2017

chanseokoh deleted the newFlexProjectWizard branch April 3, 2017 15:44


		public abstract void sendAnalyticsPing();

		public abstract IStatus validateDependencies(boolean fork, boolean cancelable);


		public abstract String getCreateNewProjectOperationLabel();

		public abstract IFile createProjectFiles(IProject newProject, AppEngineProjectConfig config,


		public abstract void addAppEngineFacet(IProject newProject, IProgressMonitor monitor) throws CoreException;

		public abstract String getCreateNewProjectOperationLabel();


		public abstract IStatus validateDependencies(boolean fork, boolean cancelable);

		public abstract CreateAppEngineWtpProject getAppEngineProjectCreationOperation(



		</plugin>

Conversation

nbashirbello commented Feb 28, 2017

Uh oh!

nbashirbello commented Feb 28, 2017

Uh oh!

nbashirbello commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chanseokoh Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briandealwis Mar 1, 2017 • edited by chanseokoh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

chanseokoh Feb 28, 2017 •

edited

Loading

briandealwis Mar 1, 2017 •

edited by chanseokoh

Loading

codecov-io commented Mar 2, 2017 •

edited

Loading