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

Ns small wizard fixes #512

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Ns small wizard fixes #512

wants to merge 4 commits into from

Conversation

natibekele
Copy link
Contributor

@natibekele natibekele commented Sep 19, 2019

small pr to fix dirty state in wizard and fix filtering on source languages


This change is Reviewable

@natibekele natibekele requested a review from jsarabia September 19, 2019 14:53
Copy link
Collaborator

@jsarabia jsarabia left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @SarabiaJ)

Copy link
Contributor

@aunger aunger left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @natibekele)


src/main/kotlin/org/wycliffeassociates/otter/jvm/app/ui/projectwizard/viewmodel/ProjectWizardViewModel.kt, line 204 at r1 (raw file):

    fun filterSourceLanguages(query: String): ObservableList<Language> =
        filterLanguages(query).filtered { filteredLanguages.contains(it) }

You already have a function with this name and a different signature. Two overloaded functions should in some sense "do the same thing", but here they don't.

The names are further confusing because with this PR you're starting to differentiate between an ObservableList created here by filterSourceLanguages and some other OL created by filterLanguages. However, as I trace the assignments backward, I find that that other filtered list (filteredLanguages, the one you're not calling "source") is created by the overloaded filterSourceLanguages function. Please rework the variable and function names so "source" always means the same thing.

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