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

feature: add single source of truth for kotlin and agp versions #102

Merged
merged 3 commits into from
Sep 22, 2021

Conversation

ikarenkov
Copy link
Collaborator

kotlinVersion = kotlinVersion,
agpVersion = agpVersion,
// we don't need check properties for exist, we read it successfully in forma configuration
kotlinVersion = properties["forma.kotlinVersion"]!!.toString(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's extract property names to constants

Copy link
Collaborator Author

@ikarenkov ikarenkov Aug 2, 2021

Choose a reason for hiding this comment

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

Constants doest allowed, extracted val

}

// TODO: actually error will not be displayed, find the way to fix it
fun errorNoProperty(propertyName: String): Nothing = error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO not the best use of Nothing here. Let's replace it with throw and custom exception

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll create custom exception, it's good idea.

Снимок экрана 2021-08-03 в 00 30 30

Declaring return type explicitly for Nothing is Kotlin limitation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I meant is - there is no need to use Nothing(calling error) throw Exception would be more obvious IMHO

file.inputStream().use { properties.load(it) }
} catch (e: Throwable) {
throw Exception(
"Cannot read gradle.properties file from root project. " +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error message is too verbose. Could be one sentence.

Can't read ${file.absolutePath}

org.gradle.vfs.watch=true
forma.kotlinVersion=1.5.10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a suggestion. Should we use separate file for forma configuration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now it doesn't metter, but if we find correct way to declare versions in single place using gradle api, we should not use custom solutions. Configuring plugins using gradle.properties looks like straight forward gradle way

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having forma properties in the separate file(e.g forma.properties) will allow us to do automated refactorings(or generate file to new projects) more easily compare to file with all possible flags

)

val kotlinVersion = properties["forma.kotlinVersion"] ?: errorNoProperty("forma.kotlinVersion")
val agpVersion = properties["forma.agpVersion"] ?: errorNoProperty("forma.agpVersion")
val kotlinVersion = properties[propertyKotlinVersion] ?: errorNoProperty(propertyKotlinVersion)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see why you need Nothing here.

properties[propertyKotlinVersion] ?: errorNoProperty(propertyKotlinVersion)

could be extracted as a separate method, doing that you'll not need fun errorNoProperty

@stepango stepango merged commit 9fdabc5 into master Sep 22, 2021
@stepango stepango deleted the karenkovigor/101 branch September 22, 2021 00:45
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

2 participants