Skip to content

cleanup generated android/build.gradle artifact #138

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

Merged
merged 5 commits into from
Sep 18, 2019

Conversation

brody4hire
Copy link
Owner

as originally proposed in PR #135 by @friederbluemle

Co-authored-by: Frieder Bluemle <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
@brody4hire brody4hire self-assigned this Sep 18, 2019
Co-authored-by: Frieder Bluemle <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
@@ -54,7 +53,8 @@ repositories {
}

dependencies {
implementation "com.facebook.react:react-native:\${safeExtGet('reactnativeVersion', '+')}"
//noinspection GradleDynamicVersion
implementation 'com.facebook.react:react-native:+' // From node_modules
Copy link
Owner Author

Choose a reason for hiding this comment

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

Suggested change
implementation 'com.facebook.react:react-native:+' // From node_modules
implementation 'com.facebook.react:react-native:+' // from node_modules

Copy link
Contributor

@friederbluemle friederbluemle Sep 18, 2019

Choose a reason for hiding this comment

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

This comment is directly taken from the default template of react-native. Instead of customizing it here, it might be better to keep it in sync. If you think lower-casing is better, should we open a PR on react-native?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Owner Author

Choose a reason for hiding this comment

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

Instead of customizing it here, it might be better to keep it in sync.

+1, keeping in sync for now, at least

If you think lower-casing is better, should we open a PR on react-native?

+1 (I will deal with this sometime later)

I did push another change to add a reference to the Facebook template source, on stable branch for 0.60.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is your project, it's really up to you how much you want these boilerplate files to diverge from the official template. :) It's good to merge, please go ahead, so I can rebase #135 - I'll be on a flight and unavailable later today ;)

@@ -86,7 +86,6 @@ def configureReactNativePom(def pom) {
}

afterEvaluate { project ->

Copy link
Owner Author

Choose a reason for hiding this comment

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

comments would be nice in this afterEvaluate block, which I don't really understand at this point

Copy link
Contributor

Choose a reason for hiding this comment

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

It is related to the different phases of Gradle. The execution phase starts after the configuration phase, and this afterEvaluate block will run in between. Read more here: https://www.oreilly.com/library/view/gradle-beyond-the/9781449373801/ch03.html

Copy link
Contributor

@friederbluemle friederbluemle left a comment

Choose a reason for hiding this comment

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

Looks good to me. You can merge this first, then I'll rebase #135

Christopher J. Brody and others added 2 commits September 18, 2019 16:35
Co-authored-by: Frieder Bluemle <[email protected]>
Co-authored-by: Christopher J. Brody <[email protected]>
@brody4hire brody4hire changed the title [...] cleanup generated android/build.gradle artifact cleanup generated android/build.gradle artifact Sep 18, 2019
@brody4hire brody4hire marked this pull request as ready for review September 18, 2019 20:47
@brody4hire
Copy link
Owner Author

I just added some comments with links for reference, for the benefit to people like me, will merge once the build is green.

@brody4hire brody4hire merged commit 7678250 into dev Sep 18, 2019
@brody4hire
Copy link
Owner Author

I just merged with one more partial change squeezed in from #137 (adding comment to the beginning of android/build.gradle), to keep PR #135 cleaner. Build should remain green.

Now looking forward to your updates to #135. As a side note, I don't really want to rush that one.

@brody4hire brody4hire deleted the android-gradle-cleanup branch September 22, 2019 21:15
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