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

Catch open once read many exception for formats which don't implement required functions #102

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

briangow
Copy link
Collaborator

This PR catches the NotImplementedError exception during the open once read many testing if a format hasn't implemented the required functions. This allows the other tests to run successfully for all formats.

Apologies for the minor formatting changes, my editor was being boisterous.

@briangow briangow requested a review from tcpan October 21, 2024 15:24
@tompollard
Copy link
Contributor

Might be a good idea to just remove the benchmark test...annoying that it keeps failing and probably not worth the effort to fix.

@tompollard
Copy link
Contributor

tompollard commented Oct 21, 2024

Might also be a good idea to add a style check to the tests to try to keep consistent formatting? It looks like black is running over all of your code (presumably on save) which is going to create a bunch of conflicts for anyone happening to be working on the same files.

@briangow
Copy link
Collaborator Author

Might be a good idea to just remove the benchmark test...annoying that it keeps failing and probably not worth the effort to fix.

I agree! We should do in another PR.

@briangow
Copy link
Collaborator Author

Might also be a good idea to add a style check to the tests to try to keep consistent formatting? It looks like black is running over all of your code (presumably on save) which is going to create a bunch of conflicts for anyone happening to be working on the same files.

We can consider adding a style check test. I am not running black in my environment. I think the style updates in this PR generally conform with PEP 8, so should be fine to move forward.

@tompollard
Copy link
Contributor

tompollard commented Oct 23, 2024

Might be a good idea to just remove the benchmark test...annoying that it keeps failing and probably not worth the effort to fix.

I agree! We should do in another PR.

Looking at this more closely, the problem is that the dependencies in requirements.txt cannot be resolved. Most likely the problem is a result of WFDB being tied to Numpy v1. See: #103

@briangow briangow force-pushed the bg_open1_readmany_exception branch from cd410e6 to a537603 Compare October 31, 2024 15:40
@briangow briangow force-pushed the bg_open1_readmany_exception branch from a537603 to cd3a6e9 Compare October 31, 2024 17:23
Copy link

Benchmark results:

No benchmarks were run.

@tompollard
Copy link
Contributor

Thanks Brian, looks good to me! Hopefully the formatting changes don't trip anyone up.

@tompollard tompollard merged commit be6880b into main Oct 31, 2024
2 checks passed
@tompollard tompollard deleted the bg_open1_readmany_exception branch October 31, 2024 17:27
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.

2 participants