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

BootTidal.hs breaking change + use of defaultConfig #1074

Closed
mindofmatthew opened this issue Apr 12, 2024 · 3 comments · Fixed by #1081
Closed

BootTidal.hs breaking change + use of defaultConfig #1074

mindofmatthew opened this issue Apr 12, 2024 · 3 comments · Fixed by #1081

Comments

@mindofmatthew
Copy link
Contributor

I just ran into a bug we didn't catch with #1059: because it removes the cFrameTimespan from the config object, the default BootTidal.hs throws an error. This is pretty minor, but has a couple implications that are worth discussing:

Is this breaking change okay?

Given that this has been in the default boot file for a long time, this is likely going to break everyone's custom BootTidal file. Personally, I think this is fine (breaking changes happen, and we can increment the version number and document migration steps accordingly), but I understand if anyone's hesitant. The backwards-compatible approach would be to include all (or most) of the new Clock configuration options in the main Config datatype, and let startStream copy values individually.

That said, perhaps this is a good release to rework the boot file generally (see #954), incorporating the changes from #1007. We should also make sure that editors are falling back to the BootFile.hs installed with Tidal (e.g. tidalcycles/vscode-tidalcycles#40).

Why don't we just use defaultConfig in the first place?

I'm also not totally sure why the default boot file updates the default config option anyway, given that the new values ({cVerbose = True, cFrameTimespan = 1/20}) are the same as the defaults. Is this desirable (perhaps so that people have example code to work from), or is it accidental or unnecessary?

@polymorphicengine and @yaxu probably have thoughts, but also interested in what others have to say!

@yaxu
Copy link
Member

yaxu commented Apr 13, 2024

Yep they were just there to make it easy to tweak.

Actually this sort of problem was solved with #1007 with a magical Sound.Tidal.Boot, but in the now retired 2.0 branch. @ejconlon are you up for redoing this magic to the current dev branch? I'd be happy to do it, but don't want to take credit for the work!

@mindofmatthew
Copy link
Contributor Author

Cool—sounds good. I was toying with it yesterday and was able to port the changes using a cherry-pick, which seems to keep the authorship intact?

I'll do a test with a different repo to double-check that that extends to release notes, etc, but also happy if Eric wants to just submit a new PR!

@ejconlon
Copy link
Contributor

If you can cherry-pick it successfully, please do! Depending on how the API has evolved you may need to adjust it. If you need help let me know.

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 a pull request may close this issue.

3 participants