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

Cleanup demo app dependencies. Add local testing instructions #107

Merged
merged 9 commits into from
Jul 10, 2024

Conversation

rajramsaroop
Copy link
Contributor

Fixes https://github.com/getditto/DittoAndroidTools/issues/104

Cleaned up the dependencies.

Also added instructions if someone wanted to test tools changes either locally or in an external project.

Added the ability to set the local version number in local.properties file (since the file doesn't get committed, less chance of something changing that we don't want). When running ./gradlew publishToMavenLocal it will use the version set there. This should NOT affect publishing to Maven or the Github action - those should work as expected.

Copy link
Member

@bplattenburg bplattenburg left a comment

Choose a reason for hiding this comment

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

LGTM

build.gradle Outdated
return prop.get(key)
}

ext.getProperty = { name, defValue ->
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was just moved around from a different gradle script, but I'm not convinced that having two similarly worded methods findProperty and getProperty are the right way to go.
I think using a more specific name like getValueFromLocalProperties might be better.

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 like that idea too, I'll update the PR.

Copy link
Contributor

@zmarkan zmarkan left a comment

Choose a reason for hiding this comment

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

It's OK - just left a comment regarding getProperty that retrieves from local.properties.

@@ -37,7 +37,9 @@ afterEvaluate {
from components.release
artifactId libraryArtifactId
groupId = 'live.ditto'
version = findProperty("LIBRARY_VERSION")

def versionPropertyName = "LIBRARY_VERSION"
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'm wondering if no LIBRARY_VERSION can be found I just set to "0.0.0" as a default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means one less step in publishing locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good shout - maybe just use a different string to avoid potential confusion with real semver? SNAPSHOT or something like that.

If you do it also keep in mind the order of precedence for properties - https://docs.gradle.org/current/userguide/build_environment.html#priority_for_configurations

Currently the pipeline injects the prop as an environment variable which has the lowest precedence according to their docs - so if you stick the hardcoded test version in gradle.properties it will always take it from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be a good change such that it won't affect the GH action/publishing to maven, but for publishing locally it would just use "SNAPSHOT" as the version?

version = findProperty("LIBRARY_VERSION") ?: "SNAPSHOT"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that should work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll update the PR and readme

@rajramsaroop rajramsaroop merged commit bbe4363 into main Jul 10, 2024
@rajramsaroop rajramsaroop deleted the rr/cleanup-demo-dependencies branch July 10, 2024 21:35
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.

Update dependencies for demo app
3 participants