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

Checker API to accept the full result object. #4

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

flpvsk
Copy link
Contributor

@flpvsk flpvsk commented Jan 29, 2018

Sometimes it's valuable for the checker to have other information about
the result under question, such as original sentences or their
attributes.

This commit changes the API of the checker to support that usecase. It'a
a breaking change, that can be avoided by extending the existent API
rather than replacing it.

Alternative proposal would be to add a different option to Markov
constructor options e.g. resultChecker: (result) => Boolean.

@coveralls
Copy link

coveralls commented Jan 29, 2018

Coverage Status

Coverage increased (+0.09%) to 98.837% when pulling a189e93 on flpvsk:checker-upgrade into e75afce on scambier:master.

@flpvsk
Copy link
Contributor Author

flpvsk commented Jan 29, 2018

This is not a ready-to-merge PR. There are still more tests and changes to the documentation needed. If in general the idea is ok – I can to do those as well.

@scambier
Copy link
Owner

Thanks for your PR. Do you have a concrete usecase example where this change can be helpful?

@flpvsk
Copy link
Contributor Author

flpvsk commented Jan 29, 2018

Yes. In a side-project I'm using checker to discard results that were using only one source of text (as opposed to using several different sources). The source of text is set in an attribute of data sentences, so I need access to that info in the checker.

@flpvsk
Copy link
Contributor Author

flpvsk commented Jan 29, 2018

Also it seems like at some point the proposed API was the intended one, judging by this test (in the master branch):

https://github.com/scambier/markov-strings/blob/master/test/test.js#L117

@scambier
Copy link
Owner

scambier commented Jan 30, 2018

Indeed, I totally forgot about that test. I'm ok with this PR, but it needs to be correctly implemented.

IMO, the "cleanest" solution would be to keep the checker(sentence) option but remove it from the docs & mark it deprecated, and add a new callback(result) (or whatever its name) that will be documented. This way, we don't break the API for existing users, and don't confuse new users with two virtually identical options or an ugly boolean switch. What do you think?

@flpvsk
Copy link
Contributor Author

flpvsk commented Jan 30, 2018

Ok, I agree that it's better to deprecate checker and add a new option. I wouldn't call the new option callback, because the word is usually associated with async APIs.filter?

@scambier
Copy link
Owner

Let's go with filter. I'll review and accept your PR once the code and tests are done, thanks :)

@flpvsk flpvsk force-pushed the checker-upgrade branch 3 times, most recently from 4c1c180 to 53f7d80 Compare January 31, 2018 10:30
@flpvsk
Copy link
Contributor Author

flpvsk commented Jan 31, 2018

I've changed the implementation and the docs, added tests. Let me know if I missed something, otherwise it's ready to be merged.

Sometimes it's valuable for the checker to have other information about
the result under question, such as original sentences or their
attributes.

This commit deprecates checker API and adds a new filter API. The
difference between the two is that checker takes string as input, while
filter takes the whole result object.
@scambier scambier merged commit a189e93 into scambier:master Jan 31, 2018
@scambier
Copy link
Owner

Merged and published :)

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

3 participants