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

feat: Functions Quickstart Kotlin #640

Merged
merged 7 commits into from
Sep 7, 2018
Merged

feat: Functions Quickstart Kotlin #640

merged 7 commits into from
Sep 7, 2018

Conversation

thatfiredev
Copy link
Member

Fixes #631

@the-dagger
Copy link
Contributor

@rosariopfernandes regarding the build failure here :
https://travis-ci.org/firebase/quickstart-android/builds/425545433#L1816

You might want to add the constraint-layout dependency explicitly to your module's build.gradle

I bumped into the same issue with the auth quickstart.
https://github.com/firebase/quickstart-android/pull/641/files#diff-8701e0aa849cdea4ec601239d512698bR44

@thatfiredev
Copy link
Member Author

Thank you very much @the-dagger I'll give this a try!

@samtstern
Copy link
Contributor

Here's what I did for the ConstraintLayout fix:
https://github.com/firebase/quickstart-android/pull/638/files#diff-29fcb4b75cb9b0af38cdf8b722cca057

I made the internal module export its dependencies as api. I think that's probably the "most correct" way but I'm not too concerned about it.

@thatfiredev
Copy link
Member Author

@the-dagger 's solution doesn't seem to have fixed it. I'll try @samtstern 's solution now.

@thatfiredev
Copy link
Member Author

There we go. Build fixed. Thanks for the tips guys

*/
class MainActivity : AppCompatActivity(), View.OnClickListener {

// [START define_functions_instance]
Copy link
Member Author

Choose a reason for hiding this comment

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

@samtstern I've noticed those are the java snippets present on the documentation. Shouldn't we move them to the snippets repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rosariopfernandes good question! For now we are going to leave them where they are. But we have stopped writing new snippets in the quickstart repo.

I am making sure that as I merge these PRs I update the doc references to point to the new paths (the auth PR moved like 50+ snippets)

Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

99% good just some style nits!

createNotificationChannel()

// Check if message contains a data payload.
if (remoteMessage!!.data.isNotEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of !! just do this above:

if (remoteMessage == null) {
  return;
}

Copy link
Member Author

@thatfiredev thatfiredev Sep 7, 2018

Choose a reason for hiding this comment

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

Actually, this was my mistake. I had annotated the remoteMessage argument as nullable. I think there's no reason for that, I'll remove the notation.

showSnackbar("Error signing in.")

val response = IdpResponse.fromResultIntent(data)
Log.w(TAG, "signIn", response!!.error)
Copy link
Contributor

Choose a reason for hiding this comment

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

response?.error maybe?

// Register the channel with the system; you can't change the importance
// or other notification behaviors after this
val notificationManager = getSystemService(NotificationManager::class.java)
notificationManager!!.createNotificationChannel(channel)
Copy link
Contributor

Choose a reason for hiding this comment

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

?.

// [START function_add_numbers]
private fun addNumbers(a: Int, b: Int): Task<Int> {
// Create the arguments to the callable function, which are two integers
val data = HashMap<String, Any>()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe use the hashMapOf syntax:

hashMapOf(
  "John" to "Doe",
  "Jane" to "Smith"
)

// [START function_add_message]
private fun addMessage(text: String): Task<String> {
// Create the arguments to the callable function.
val data = HashMap<String, Any>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Same nit here about the map

@samtstern
Copy link
Contributor

Will merge after build

@samtstern samtstern merged commit 5c9982d into firebase:master Sep 7, 2018
@thatfiredev thatfiredev deleted the functions-kotlin branch September 7, 2018 21:52
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.

3 participants