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

Allow endless recording #32

Merged
merged 3 commits into from
Oct 16, 2024
Merged

Conversation

henrygerardmoore
Copy link
Collaborator

@henrygerardmoore henrygerardmoore commented Oct 15, 2024

Resolves #31 and doesn't require users to change their usage really at all, just adds 0 as a sentinel value.

Also takes care of this issue I saw in pre-commit:

[WARNING] repo `https://github.com/pre-commit/pre-commit-hooks` uses deprecated stage names (commit, push) which will be removed in a future version.  Hint: often `pre-commit autoupdate --repo https://github.com/pre-commit/pre-commit-hooks` will fix this.  if it does not -- consider reporting an issue to that repo.

My question regarding this change is: should we have the default reset time be 0? As mentioned in #31, I can understand wanting a different default behavior. I also understand not wanting to fill the disk super quickly though, especially since there's no way to ensure a user sees a warning in the docstring. I am open to other suggestions on this, but thought I'd make this PR just to start discussion.

Some other possibilities I considered:

  • Still reset the recording, but with incrementing filenames (as mentioned in Automatically RestartRecording - Questionable default behavior #31). Though this is easy to do, I opted to make the PR as-is since it is the smallest code change.
  • Change the default to 'swap' between two recordings, so you have at most 2x reset_time of data used. This way, when you reset, you don't just delete all the data. You always have at least the last reset_time data saved. Perhaps a more forgiving default behavior, but maybe a little odd

@henrygerardmoore henrygerardmoore changed the title Default increment filename Allow endless recording Oct 15, 2024
@henrygerardmoore henrygerardmoore merged commit 6b35e26 into main Oct 16, 2024
10 checks passed
@henrygerardmoore henrygerardmoore deleted the default_increment_filename branch October 16, 2024 15:15
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.

Automatically RestartRecording - Questionable default behavior
2 participants