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

Added tests for severity #1

Merged
merged 5 commits into from
Nov 3, 2016
Merged

Added tests for severity #1

merged 5 commits into from
Nov 3, 2016

Conversation

Hekku2
Copy link

@Hekku2 Hekku2 commented Nov 3, 2016

  • Added tests for related items
  • Fixed broken tests
  • Fixed jshint-warnings
  • Fixed case where observable with severity-function defined causes errors. This can happen when for example 'equal'-rule is used and it refers to validatable observable.

Fixed jshint-warnings
Fixed case where observable with severity-function defined causes errors. This can happen when for example 'equal'-rule is used and it refers to validatable observable.
@egucciar
Copy link

egucciar commented Nov 3, 2016

It seems like all of the use cases of severity were covered by your tests.

However I did notice you added an extra conditional check to ensure params being passed to KO validation is not observable. Is this needed?

There are cases where I am passing observables to KO validation for custom rules I have created in order to do more complex validations which require reacting to changes in params. Let me know if you can resubmit the pull request without this check, and if so I will merge it.

@Hekku2
Copy link
Author

Hekku2 commented Nov 3, 2016

That indeed breaks that.

The problem that rises is that now 'validatable' observables can't be directly used as parameters, because they now have the 'severity'-function added by this feature. I belive this breaks current functionality.

Perhaps the severity-function in validatable should be renamed to something different, like 'errorSeverity' or something?

See commit for suggestion.

Removed 'isObservable'-check, because it is not needed anymore.
@egucciar
Copy link

egucciar commented Nov 3, 2016

due to the fact we are using this lib in many places already I am hesitant to introduce a breaking change to our code.

Let's think of this from another perspective, in your first commit you disregard all params which are observables, that didn't work for my use case, but I don't have a usecase where I actually require params to be a validated observable. Why not check if the severity property is observable and return only then? That way both our usecases get satisfied without having to introduce breaking API changes within the code.

params && !ko.isObservable(params.severity)

@Hekku2
Copy link
Author

Hekku2 commented Nov 3, 2016

That sounds fine.

@egucciar
Copy link

egucciar commented Nov 3, 2016

Let me know once the commits are made.

@Hekku2
Copy link
Author

Hekku2 commented Nov 3, 2016

Now it should not break existing code, hoping that someone doesn't use severity observable with parameters.

@egucciar egucciar merged commit 20a6090 into EikosPartners:master Nov 3, 2016
@egucciar
Copy link

egucciar commented Nov 3, 2016

I agree, I will have to think about how we should be handling that down the line, but for now this is great. thanks for your contribution.

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.

2 participants