[GitHub] [cordova-android] breautek commented on a change in pull request #1173: Feature: Write name of the Android app to .idea/.name for Android Studio

[GitBox <gi...@apache.org>]

breautek commented on a change in pull request #1173:
URL: https://github.com/apache/cordova-android/pull/1173#discussion_r601641401



##########
File path: bin/lib/create.js
##########
@@ -197,6 +198,19 @@ function validateProjectName (project_name) {
     return Promise.resolve();
 }
 
+/**
+ * Write the name of the app in "platforms/android/.idea/.name" so that Android Studio can show that name in the
+ * project listing. This is helpful to quickly look in the Android Studio listing if there are so many projects in
+ * Android Studio.
+ *
+ * https://github.com/apache/cordova-android/issues/1172
+ */
+function writeNameForAndroidStudio (project_path, project_name) {
+    var ideaPath = path.join(project_path, '.idea');

Review comment:
       ```suggestion
       const ideaPath = path.join(project_path, '.idea');
   ```

##########
File path: spec/unit/create.spec.js
##########
@@ -300,4 +301,23 @@ describe('create', function () {
             });
         });
     });
+
+    describe('writeNameForAndroidStudio', () => {
+        var project_path = path.join('some', 'path');

Review comment:
       In general we want to avoid using `var`, and instead we use `let` or `const` depending on if we expect the variable to change.
   
   ```suggestion
           const project_path = path.join('some', 'path');
   ```

##########
File path: spec/unit/create.spec.js
##########
@@ -300,4 +301,23 @@ describe('create', function () {
             });
         });
     });
+
+    describe('writeNameForAndroidStudio', () => {
+        var project_path = path.join('some', 'path');
+
+        it('should call ensureDirSync with path', () => {
+            spyOn(fs, 'ensureDirSync');
+            spyOn(fs, 'writeFileSync');
+            create.writeNameForAndroidStudio(project_path, 'Test Android');
+            expect(fs.ensureDirSync).toHaveBeenCalledWith(path.join(project_path, '.idea'));
+        });
+
+        it('should call writeFileSync with content', () => {
+            var appName = 'Test Cordova';
+            spyOn(fs, 'ensureDirSync');
+            spyOn(fs, 'writeFileSync');

Review comment:
       See `beforeEach` usage above
   
   ```suggestion
   ```

##########
File path: spec/unit/create.spec.js
##########
@@ -300,4 +301,23 @@ describe('create', function () {
             });
         });
     });
+
+    describe('writeNameForAndroidStudio', () => {
+        var project_path = path.join('some', 'path');
+
+        it('should call ensureDirSync with path', () => {
+            spyOn(fs, 'ensureDirSync');
+            spyOn(fs, 'writeFileSync');

Review comment:
       See `beforeEach` usage above.
   
   ```suggestion
   ```

##########
File path: spec/unit/create.spec.js
##########
@@ -300,4 +301,23 @@ describe('create', function () {
             });
         });
     });
+
+    describe('writeNameForAndroidStudio', () => {
+        var project_path = path.join('some', 'path');
+
+        it('should call ensureDirSync with path', () => {
+            spyOn(fs, 'ensureDirSync');
+            spyOn(fs, 'writeFileSync');
+            create.writeNameForAndroidStudio(project_path, 'Test Android');
+            expect(fs.ensureDirSync).toHaveBeenCalledWith(path.join(project_path, '.idea'));
+        });
+
+        it('should call writeFileSync with content', () => {
+            var appName = 'Test Cordova';

Review comment:
       Same as above
   
   ```suggestion
               const appName = 'Test Cordova';
   ```

##########
File path: spec/unit/create.spec.js
##########
@@ -300,4 +301,23 @@ describe('create', function () {
             });
         });
     });
+
+    describe('writeNameForAndroidStudio', () => {
+        var project_path = path.join('some', 'path');

Review comment:
       We can reduce some code duplication by using the `beforeEach` test hook, which gets ran before each test.
   
   ```suggestion
           beforeEach(() => {
               spyOn(fs, 'ensureDirSync');
               spyOn(fs, 'writeFileSync');
           });
           var project_path = path.join('some', 'path');
   ```
   
   Then in each individual test inside this describe block, those two methods are already spied on, so we can remove those `spyOn` lines in the individual tests.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org