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

[WIP] [FIX] New signals: default output #2390

Closed
wants to merge 2 commits into from
Closed

[WIP] [FIX] New signals: default output #2390

wants to merge 2 commits into from

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Jun 8, 2017

Issue

screenshot_20170608_152918
Selected Data should be default instead of Features.

screenshot_20170608_153216
Data should be default instead of Features.

Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@codecov-io
Copy link

codecov-io commented Jun 8, 2017

Codecov Report

Merging #2390 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #2390      +/-   ##
=========================================
+ Coverage   73.88%   73.9%   +0.01%     
=========================================
  Files         322     322              
  Lines       55843   55843              
=========================================
+ Hits        41262   41270       +8     
+ Misses      14581   14573       -8

@janezd
Copy link
Contributor

janezd commented Jun 23, 2017

As far as I recall, we said that the problem here is that Data became a dynamic signal, and static signals have precedence over dynamic.

@ales-erjavec, I had an idea that we could change the signal connection policy so that when the signal is dynamic, it still has the same priority as static if the types actually match. That is, if Table is connected to Table (not to Corpus) it remains the default signal although it's dynamic (because it would match even if it was static). I'm not sure I still like it; it's messy. Do you have a better idea?

Another option is to eliminate non-dynamic signals. There are currently almost no static signals. @astaric?

At any rate, while this modification fixes the problem, I don't think it fixes it correctly. If we accept it, we should at least add a comment why the change was necessary.

@ales-erjavec
Copy link
Contributor

@ales-erjavec, I had an idea that we could change the signal connection policy so that when the signal is dynamic, it still has the same priority as static if the types actually match. That is, if Table is connected to Table (not to Corpus) it remains the default signal although it's dynamic (because it would match even if it was static). I'm not sure I still like it; it's messy. Do you have a better idea?

I think this is the best solution. Please see #2431

@janezd
Copy link
Contributor

janezd commented Jun 23, 2017

Closed in favor of #2431.

@janezd janezd closed this Jun 23, 2017
@jerneju jerneju deleted the new-signals-default branch June 26, 2017 07:24
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.

4 participants