Add hooks in quarks
Purpose and Motivation
Implements #5753 Before writing docs and tests I want to make sure that this implementation is fine.
I switched form upgrade->update and remove->uninstall terminology in order to reflect more the terminology used within SuperCollider than in pacman.
Types of changes
- Documentation
- New feature
To-do list
- [x] Code is tested
- [ ] All tests are passing
- [x] Updated documentation
- [x] This PR is ready for review
Thanks for the feedback @scztt - do you think the additional commits are sufficient? if so, I'll start to write docs for it :)
Updated docs
Added tests, but not all tests are passing, e.g.
FAIL: a TestCoreUGens: test_ugen_generator_equivalences - "Duty.ar(SampleDur.ir, 0, x) == x"
1 of 48000 items in array failed to match. Displaying arrays from index of first failure (0) onwards:
FloatArray[ 0.60310339927673, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0....etc...
! =
0
but it seems this was not broken by this PR
Anything thats missing from a merge?
@scztt - how are you feeling with the changes?
Should this be considered for https://github.com/supercollider/supercollider/milestone/24 ? Makes sense to me to include it in an major version upgrade, making it easy for checking if this hook functionality is existing.
@scztt just checking in, would you be able to follow up on your review?
@capital-G I saw that scott had some comments in the review – have you checked them?
Bump
@capital-G
This COULD be a problem, sadly...
git.checkouthere can change the Quark data, and this potentially change the hooks. It MIGHT be that what we actually want to do here is:1. Run `\preUpdate` 2. `git.pull()` / `git.checkout` 3. Reload `data` 4. run `\postUpdate`It would probably have to be clearly documented that
\preUpdateis always the OLD hook, and\postUpdateis always the NEW hook.Without doing this, we'd only ever be running the old post hook, which seems wrong.
Yes, I documented this behavior, see this comment https://github.com/supercollider/supercollider/pull/5780#issuecomment-1140453715
BTW we are planning to do a release candidate for 3.13 this weekend. I'll add this to the milestone so that it doesn't fall off the radar.
@dyfer seems like the CI has completed, so you can resolve that request.
@telephon The test suite job in GHA took 1 minute to finish, which unfortunately means that it exited (successfully...) without actually running the tests. I've seen that happening before. I'm sorry this is so unreliable, but I was unsuccessful in making it better so far.
Because of this I have built this PR locally and noticed that some tests fail. If I missed something, please let me know.
The tests were broken when I created the branch - should i merge or rebase (what is better in this scenario?) the main branch, this should trigger the test suite with the proper changes right - if it works its fine, if it fails than it shouldnt be merged.
@capital-G Please take a look at my message above. I've only run tests added in this PR (TestQuark) and found failures there. Other failures of course are unrelated and would not block merging this PR. Can you please check the tests added here? If they only fail on my machine but not on yours, then we'd need to investigate further.
Oh, I oversaw the details section, thanks for the hint, will fix this.
@capital-G why did you close this? I think the tests probably just make some wrong assumption. If you like, we can look through it.
See bottom of https://github.com/supercollider/supercollider/pull/5487#issuecomment-1312736678 If someone else wants to continue/finish this please do so, the branch is still existing.
BTW we are planning to do a release candidate for 3.13 this weekend. I'll add this to the milestone so that it doesn't fall off the radar.
@dyfer depening on the schedule, maybe you could add #5907 to the milestone for 3.13 ?
@dyfer depening on the schedule, maybe you could add #5907 to the milestone for 3.13 ?
@telephon yes, I was planning to do that. Since we just did 3.13.0-rc1, I'm trying to decide whether we should make 3.13.0-rc2 with that, or should that PR wait until 3.13.1. Technically I think new functionality shouldn't be added from one rc to the next, but this is relatively small and I was hoping it would be included... So, I'm planning for #5907 to go into 3.13.1 at the latest, but possibly earlier.