Skip to content
This repository has been archived by the owner on Jan 22, 2020. It is now read-only.

Cleanup. #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Cleanup. #19

wants to merge 4 commits into from

Conversation

IAmAnubhavSaini
Copy link

In app/index.js:

variable renaming.
comment removal.
readability touchups.

@@ -66,8 +62,8 @@ var NemoGenerator = yeoman.generators.Base.extend({
fs.writeFileSync('Gruntfile.js', gruntContent);
}
//let's update Gruntfile.js if possible
var gruntfileData = fs.readFileSync('Gruntfile.js'),
output = gruntFileApi.init(gruntfileData);
var gruntfileData = fs.readFileSync('Gruntfile.js');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say, if you are going to remove this grouped var declaration, please can you do the same throughout this file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@grawk
Copy link
Member

grawk commented Jun 27, 2016

What would make this a valuable PR, in my opinion, would be if you could go through and hoist any hanging var declarations to the top of their scope. I noticed that some declarations are present lower in some blocks while reviewing this PR.

variable renaming.
comment removal.
readability touchups.
@IAmAnubhavSaini
Copy link
Author

Sorry for late response; got busy with PPWC.

Merging two separate code branching ifs.
Creating cleaner copy functions.
@gabrielcsapo
Copy link

@grawk is this still viable since we merged this in (#23)?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants