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

Only add google to build.grade if it is missing. #894

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brendoncoin
Copy link

For fixing issue #893.

Copy link
Contributor

@briantq briantq left a comment

Choose a reason for hiding this comment

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

For consistency, we should do the same checks for the Fabric repo too.

var googlesMavenRepo = whitespace + 'google() // Google\'s Maven repository from cordova-plugin-firebase';
// Only add Google's Maven repo if it's not already there
var googleRepoAlreadyExists = buildGradle.test(/(google\(\))|(https:\/\/maven\.google\.com)/m);
if (googleRepoAlreadyExists) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of setting the variable twice, along with running regex and string replacement, why don't we wrap the change to only run the code if !googleRepoAlreadyExists? This will reduce code complexity and improve performance.

@briantq
Copy link
Contributor

briantq commented Oct 7, 2018

@brendoncoin I am in support of your approach, but want to make sure we simplify the code/improve build performance along with being consistent with Fabric. Additionally, there are some tests failing on the specific lines you modified (referencing the test method). Could you take a look as we need to get those passing before we merge the code.

@briantq
Copy link
Contributor

briantq commented Oct 17, 2018

@brendoncoin if you would like to get this merged please address the comments and check the build failure. I am happy to answer any questions as I definitely enjoy merging code more so than closing inactive pull requests.

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.

2 participants