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

Adding has_group and has_channel #314

Merged
merged 5 commits into from
Nov 5, 2023
Merged

Conversation

looopTools
Copy link
Contributor

I am in a situation where I am not fully aware of what groups and channels are in which TDMS file.
Therefore we found it beneficial to have a function to ask if a TdmsFile has a group and ask if a TdmsGroup has a channel.

I though it could be beneficial to others as well

Copy link
Owner

@adamreeve adamreeve left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @looopTools, I've left a couple of comments for consideration

nptdms/tdms.py Outdated
@@ -145,6 +145,17 @@ def groups(self):

return list(self._groups.values())

def has_group(self, group_name):
Copy link
Owner

Choose a reason for hiding this comment

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

This seems like a good use case for implementing the __contains__ method, so rather than using tdms_file.has_group('group_name') you could use 'group_name' in tdms_file, which I think would be more idiomatic Python.

In fact it looks like that second syntax already works due to TdmsFile and TdmsGroup implementing the __iter__ method, but it probably makes sense to provide a more efficient implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment.
I agree I will see if I can get it working either today or Saturday :)

nptdms/tdms.py Outdated
Comment on lines 153 to 157
try:
self._groups[group_name]
except KeyError:
return False
return True
Copy link
Owner

Choose a reason for hiding this comment

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

This can be simplified to return group_name in self._groups

@looopTools
Copy link
Contributor Author

I changed to using __contains__ on TdmsGroup and then I added two test one to test that in actually worked on TdmsFile and one to test if in worked on TdmsGroup

@adamreeve
Copy link
Owner

Thanks. Was it intentional to not implement __contains__ in TdmsFile too though? Although in does work there too I think it makes sense to have an explicit (and presumably faster) __contains__ implementation for both TdmsFile and TdmsGroup

@looopTools
Copy link
Contributor Author

I will implement that too :)

@looopTools
Copy link
Contributor Author

Added Contains on TDMS File as well

@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ea567c8) 96.89% compared to head (b5fefa7) 96.89%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #314   +/-   ##
=======================================
  Coverage   96.89%   96.89%           
=======================================
  Files          19       19           
  Lines        2606     2610    +4     
=======================================
+ Hits         2525     2529    +4     
  Misses         81       81           
Files Coverage Δ
nptdms/tdms.py 97.84% <100.00%> (+0.02%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@adamreeve adamreeve left a comment

Choose a reason for hiding this comment

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

👍 thanks!

@adamreeve adamreeve merged commit 58b9808 into adamreeve:master Nov 5, 2023
14 checks passed
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.

3 participants