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

Add option to remove channels from ChannelList #1045

Conversation

Dominik-Vogel
Copy link
Contributor

@Dominik-Vogel Dominik-Vogel commented Apr 11, 2018

Tested and seems to not cause problems - travis test fails... It seems to be a mypy issue. Cannot know if it is a tuple or not.

@Dominik-Vogel
Copy link
Contributor Author

Hang on I just noticed the test that is not passing here...

@jenshnielsen
Copy link
Collaborator

jenshnielsen commented Jun 13, 2018

@Dominik-Vogel I would suggest merging #1136 first which cleans up the typing of the channellist you may also need to cast the _channels to an array after the instance check.

@Dominik-Vogel
Copy link
Contributor Author

Thank you @jenshnielsen, this is a great suggestion!

@jenshnielsen
Copy link
Collaborator

By casting to an array l mean something like

channels = cast(List[InstrumentChannel], self._channels)

Which does not mention array at all, sorry for the confusion

@@ -263,6 +263,18 @@ def append(self, obj: InstrumentChannel):
self._channel_mapping[obj.short_name] = obj
return self._channels.append(obj)

def remove(self, obj: InstrumentChannel):
Copy link
Contributor

Choose a reason for hiding this comment

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

no testing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should probably have a test but was not ready to merge anyway. Somehow got pushed to master anyway by accident with failing test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added extensive testing and this should be good to go now, @jenshnielsen, @astafan8

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@codecov
Copy link

codecov bot commented Jun 14, 2018

Codecov Report

Merging #1045 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #1045     +/-   ##
=========================================
+ Coverage   79.64%   79.75%   +0.1%     
=========================================
  Files          46       46             
  Lines        6632     6638      +6     
=========================================
+ Hits         5282     5294     +12     
+ Misses       1350     1344      -6

@jenshnielsen jenshnielsen merged commit bf2afed into microsoft:master Jun 14, 2018
giulioungaretti pushed a commit that referenced this pull request Jun 14, 2018
Merge: 45b91fc 25272af
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>

    Merge pull request #1045 from Dominik-Vogel/feat/remove_channels_from_channellist
@Dominik-Vogel Dominik-Vogel deleted the feat/remove_channels_from_channellist branch November 27, 2018 15:35
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