Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CB-11117: Use FileUpdater to optimize prepare for android platform #295

Closed
wants to merge 6 commits into from
Closed

Conversation

jasongin
Copy link
Contributor

This uses the FileUpdater module added in apache/cordova-lib#429 to optionally skip copying files that didn't change. Some refactoring was required because previously the target directories would just be wiped before copying; now we need to map out the source and target directories so the FileUpdater has the necessary information to determine the optimal set of file operations.

@jasongin
Copy link
Contributor Author

@vladimir-kotikov @infil00p please review

@codecov-io
Copy link

codecov-io commented Apr 27, 2016

Current coverage is 34.14%

Merging #295 into master will decrease coverage by <.01%

@@             master       #295   diff @@
==========================================
  Files            11         11          
  Lines           947        949     +2   
  Methods         194        195     +1   
  Messages          0          0          
  Branches        153        153          
==========================================
  Hits            324        324          
- Misses          623        625     +2   
  Partials          0          0          

Powered by Codecov. Last updated by d125ece...be92dce


this._config = updateConfigFilesFrom(cordovaProject.projectConfig,
this._munger, this.locations);

// Update own www dir with project's www assets and plugins' assets and js-files
return Q.when(updateWwwFrom(cordovaProject, this.locations))
return Q.when(updateWww.call(self, cordovaProject))
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not to pass this reference to this method, especially if the only thing we need is a couple of properties from locations object (this.locations.platformWww and this.locations.www at L120). Maybe just pass these properties as an arguments, or revert to previous signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had been using this in a few more places, before I moved some things around. I agree it is not really needed now; I'll clean that up everywhere.

@jasongin
Copy link
Contributor Author

jasongin commented May 2, 2016

I updated the PR based on feedback. As mentioned in the other comment I've kept the clean functionality, but it is now skipped when not invoked via the CLI.

@vladimir-kotikov
Copy link
Member

LGTM

@asfgit asfgit closed this in 3a1b4ff May 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants