agoda-com / Kakao

This repo is no longer supported. Please visit a https://github.com/KakaoCup/Kakao
Apache License 2.0
1.11k stars 102 forks source link

Add hasHint(@StringRes resId: Int) #217 #218

Closed igorwojda closed 4 years ago

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

Unlimity commented 4 years ago

@igorwojda no, you're not missing anything. Actually, I would also recommend to add resource id match for error as well.

Vacxe commented 4 years ago

@igorwojda Yes mate. All good! Maybe will be better to add a new function like

    /**
     * Checks if this input layout has given hint
     *
     * @param resId - hint text resource to be checked
     */
    fun hasHint(@StringRes resId: Int) {
        view.check(ViewAssertion { view, notFoundException ->
            if (view is TextInputLayout) {
                val expectedHint = view.context.getString(resId)
                if (expectedHint != view.hint) {
                    throw AssertionError(
                        "Expected hint is $expectedHint," +
                                " but actual is ${view.hint}"
                    )
                }
            } else {
                notFoundException?.let { throw AssertionError(it) }
            }
        })
    }

WDYT @igorwojda @Unlimity ?

igorwojda commented 4 years ago

@Unlimity I have added equivalent method for error fun hasError(@StringRes resId: Int)

igorwojda commented 4 years ago

@igorwojda Yes mate. All good! Maybe will be better to add a new function like

    /**
     * Checks if this input layout has given hint
     *
     * @param resId - hint text resource to be checked
     */
    fun hasHint(@StringRes resId: Int) {
        view.check(ViewAssertion { view, notFoundException ->
            if (view is TextInputLayout) {
                val expectedHint = view.context.getString(resId)
                if (expectedHint != view.hint) {
                    throw AssertionError(
                        "Expected hint is $expectedHint," +
                                " but actual is ${view.hint}"
                    )
                }
            } else {
                notFoundException?.let { throw AssertionError(it) }
            }
        })
    }

WDYT @igorwojda @Unlimity ?

I don't really see any benefit of doing so (instead of using one overload with in another, like in this PR).

However on the other side we have a big downside of heaving two full/long implementations - higher maintenance cost.

Vacxe commented 4 years ago

@igorwojda awesome! Thanks a lot! Last request from me. Could u please extract


private fun getResourceString(@StringRes resId: Int) = InstrumentationRegistry.getInstrumentation().targetContext.let { it.resources.getString(resId) }

To

common/utility/ContextUtils.kt
igorwojda commented 4 years ago
  1. I will name it utilities (plular) to match other package names

  2. Should docs/kakao be a apart of this PR?

image

Vacxe commented 4 years ago

@igorwojda don't worry about notes. This PR could contain only code related stuff. Cheers.

igorwojda commented 4 years ago

done

Vacxe commented 4 years ago

@igorwojda thanks for the contribution