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

Refactor header fields #42

Merged
merged 7 commits into from
Apr 6, 2024
Merged

Refactor header fields #42

merged 7 commits into from
Apr 6, 2024

Conversation

hofaflo
Copy link
Contributor

@hofaflo hofaflo commented Mar 29, 2024

The intention behind the current implementation of header fields in the Edf and EdfSignal classes, based on RawHeaderFields, was to avoid bugs caused by having to repeat the field length everywhere a header field is set. However, it comes with a few issues:

  • there are several places where #type: ignore comments are needed to make mypy happy, because some non-public attributes are created via setattr
  • it is difficult to create subclasses with differing behavior, which is required for implementing Refactor EdfSignal class #34 and for supporting BDF
  • the idea to keep the raw header fields as bytes in non-public variables of the same name can't be followed consistently, if custom getters and setters are needed (e.g. for EdfSignal.label, Edf.startdate, Edf.starttime)
  • it could be difficult for new contributors to grasp the nested logic and "magic"

I think these issues outweigh the initially desired benefit and would therefore like to replace the custom descriptors with ordinary properties.

@hofaflo hofaflo requested a review from marcoross March 29, 2024 09:02
@hofaflo hofaflo self-assigned this Mar 29, 2024
@hofaflo hofaflo marked this pull request as draft March 29, 2024 09:20
@hofaflo hofaflo marked this pull request as ready for review March 29, 2024 09:27
Base automatically changed from read-edf-with-invalid-filesize to main March 29, 2024 09:40
marcoross
marcoross previously approved these changes Apr 6, 2024
@hofaflo
Copy link
Contributor Author

hofaflo commented Apr 6, 2024

Resolved merge conflict, @marcoross please re-approve :)

@hofaflo hofaflo merged commit abdac14 into main Apr 6, 2024
9 checks passed
@hofaflo hofaflo deleted the refactor-header-fields branch April 6, 2024 09:58
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