-
Notifications
You must be signed in to change notification settings - Fork 120
Several changes to the read() and write() methods (alternative) #37
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
Closed
Closed
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
1f22833
Add open(), read() and write() functions
mgeier 4258760
read()/write() overhaul
mgeier 202fe2b
Add arguments channels_first, always_2d, out and fill_value
mgeier 5d7236c
Change handling of out argument
mgeier f64da11
Add helper function _check_frames_and_channels()
mgeier ace9936
refactored tests for improved clarity
bastibe 49a16c8
Simplified read and write
bastibe db2cb0b
refactored `read` to accept `out`
bastibe a700fcf
re-integrated read and readinto
bastibe 161f97e
Fixed documentation of test case
bastibe 94c2a00
Added tests for out argument of read
bastibe 564e9bc
removed duplicated self.seek(0, SEEK_CUR, 'r')
bastibe 1a154da
no more reshaping out after creation
bastibe 4d28a10
no more allocating of unused memory
bastibe 28527a7
no more returning views of all the data
bastibe 5c68a04
removed unnecessary nesting
bastibe 877fc3a
fixed write function
bastibe b56606e
fixed write function for real and test it
bastibe 1d3a217
some refactorings to improve clarity
bastibe 6f43d1c
fixed broken docstring
bastibe d331c89
added test for read with empty out
bastibe 06599da
renamed read_frames to frames_to_read
bastibe 7471637
Added checks for out dimensionality
bastibe 042d425
refactored error checking into its own method
bastibe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 don't quite like this if statement.
I think mine was easier to understand:
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 disagree. The nice thing about my version is that it does not need nested logic. The two pieces of code are functionally equivalent, but my code makes it clear that there are three branches, whereas your code "hides" the first else.
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 this is largely (but not entirely) a matter of taste.
Let's compare directly:
vs.
Both are functionally equivalent (if I'm not mistaken) except in the case that
len(out) < frames, which I guess can never happen (at least it shouldn't).I think the nested logic actually helps understanding, in your suggestion the nested logic is somehow "folded into" the
elifcondition.I think the second example follows exactly my understanding (but probably only mine) about what's going on:
outoutis larger thanframes), we changeout, otherwise we don't do anything with itout(by either "cutting off" the rest or by filling it with somethingoutI think this is a very common pattern: changing something if a certain condition is met, doing nothing if not.
This is also the reason why the
elsepart of anifstatement is optional in probably all languages that have anifstatement [citation needed].In addition to having a less clear (IMHO) logic, the first example has also a code smell because common functionality (
returning something) is repeated in each branch.