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

[FIX] OWPreprocess: commit automatically for N-grams #409

Closed
wants to merge 1 commit into from

Conversation

matejklemen
Copy link
Contributor

Issue

Fixes #336.

Description of changes

Changed the connection of signal so that it is consistent with how it's done in FilteringModule (the other place where RangeWidget is used).

Includes
  • Code changes
  • Tests
  • Documentation

@ajdapretnar
Copy link
Collaborator

This works well! Thanks!
Could you perhaps add some tests? 🤔
I will restart Travis, because it doesn't seem to exit on this issue.

@matejklemen
Copy link
Contributor Author

Sorry for the late reply - I'll try to figure something out for tests when I find some time

@ajdapretnar
Copy link
Collaborator

Sure, no worries. 👍
Could you also rebase to latest master so that the tests would pass? We fixed a couple of things by now.

@s-alexey
Copy link
Collaborator

s-alexey commented Apr 10, 2019

It seems like there is a problem with RagneWidget.valueChanged signal as well. I think we could drop it as no longer used.

@codecov-io
Copy link

codecov-io commented Jun 14, 2019

Codecov Report

Merging #409 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #409   +/-   ##
=======================================
  Coverage   85.96%   85.96%           
=======================================
  Files          34       34           
  Lines        1931     1931           
  Branches      331      331           
=======================================
  Hits         1660     1660           
  Misses        230      230           
  Partials       41       41

@ajdapretnar
Copy link
Collaborator

I am sorry @matejklemen for the mess I have made today. Especially messing with your branch. Apologies! 🙏

I made a new PR with your commit in #436. If tests pass, I will merge it. You remain the author of the code.

Thank you for this fix!

@ajdapretnar
Copy link
Collaborator

Continued in #436.

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.

Preprocess Text: N-grams doesn't commit automatically
4 participants