-
Notifications
You must be signed in to change notification settings - Fork 2
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 tests for string markers #14
base: main
Are you sure you want to change the base?
Conversation
Can you rebase please? I assume this fixes #10, correct? |
It does, but listen, can we go one by one? As I said, so many PRs add complexity for me. I don't want to keep three or four branches at the same time. Which one would you like to do first? Should we go for this one now? |
Yes, let's do this one now. |
* upstream/main: Add Autoformatting with Blue style (cbrnr#12)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
### ✨ Added | ||
- Add tests for string markers from issue xdf-modules/libxdf#19 (([#13](https://github.com/cbrnr/XDF.jl/pull/13) by [Alberto Barradas](https://github.com/abcsds))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be more specific here. You fixed an issue with string streams consisting of multiple channels, right? It would be more helpful to explicitly mention this here instead of linking to an issue. Also, I think this change should then go into the "Fixed" section, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What did you mention is the template to follow? I can't find the emoji for the fixed section. I thought you mentioned a repo from xdf-modules, but their changelogs don't have emojis. I can't find the pr where it was mentioned. I thought it was #11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be up for adding this file to https://github.com/xdf-modules/example-files/ instead? That way, other projects could also use this test file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds tests for file from xdf-modules/libxdf#19 Small EEG xdf with two channel string markers. We do not present the issue. Tests pass.