Review 3 of forgejo/forgejo!12512 #3

oliverpool commented 2026年06月13日 22:05:55 +02:00

I realized that the call refactor dropped the check number of args verification that was previously present.

I added it back via a staticArgs field.

I also reverted the change of passing the whole signature (it is only passing the arguments), since the function name can be computed using fun.

---

I then wondered why permissions was passed to call: I initially thought that this was a hint at the need of structured fixtureData... however when digging, I realized that this argument wasn't used... so I dropped it.

---

typos-lsp found a pasword typo.

---

I have run some basic cpuprofiling on the tests:

• the test run 25% faster (15s instead of 20s) if we disable the fixture insertion (only keep the fixture cleanup). Maybe a unittest.ClearFixtureTables() method would be a good investment
• somehow of the remaining 15s, my cpuprofile shows only 5s. The main time seems spent blocking somewhere, but I wasn't able to pinpoint where exactly. Maybe this is worth some further digging.

---

Overall I think that the current shape (with or without the present suggestions) is in a shape that I would be able to maintain forward!

I realized that the `call` refactor dropped the `check number of args` verification that was previously present. I added it back via a `staticArgs` field. I also reverted the change of passing the whole signature (it is only passing the arguments), since the function name can be computed using `fun`. --- I then wondered why `permissions` was passed to `call`: I initially thought that this was a hint at the need of structured fixtureData... however when digging, I realized that this argument wasn't used... so I dropped it. --- [typos-lsp](https://github.com/tekumara/typos-lsp) found a `pasword` typo. --- I have run some basic cpuprofiling on the tests: - the test run 25% faster (15s instead of 20s) if we disable the fixture insertion (only keep the fixture cleanup). Maybe a `unittest.ClearFixtureTables()` method would be a good investment - somehow of the remaining 15s, my cpuprofile shows only 5s. The main time seems spent blocking somewhere, but I wasn't able to pinpoint where exactly. Maybe this is worth some further digging. --- Overall I think that the current shape (with or without the present suggestions) is in a shape that I would be able to maintain forward!

limiting-factor commented 2026年06月14日 07:45:35 +02:00

@oliverpool wrote in #3 (comment):

I realized that the call refactor dropped the check number of args verification that was previously present.
I added it back via a staticArgs field.

It is better. I thought about adding it but did not because the call being responsible for calling the more elaborate functions makes it less of a debug problem.

I also reverted the change of passing the whole signature (it is only passing the arguments), since the function name can be computed using fun.

Better, good call.

I then wondered why permissions was passed to call: I initially thought that this was a hint at the need of structured fixtureData... however when digging, I realized that this argument wasn't used... so I dropped it.

IIRC I initially added it because I was not sure if it would be needed. But since it is currently not used, better not have it. Adding it later won't be a major refactor.

typos-lsp found a pasword typo.

That exists also in services/auth/method but that's for another time 😄

@oliverpool wrote in https://codeberg.org/limiting-factor/forgejo/pulls/3#issue-5800472: > I realized that the `call` refactor dropped the `check number of args` verification that was previously present. > I added it back via a `staticArgs` field. It is better. I thought about adding it but did not because the `call` being responsible for calling the more elaborate functions makes it less of a debug problem. > I also reverted the change of passing the whole signature (it is only passing the arguments), since the function name can be computed using `fun`. Better, good call. > I then wondered why `permissions` was passed to `call`: I initially thought that this was a hint at the need of structured fixtureData... however when digging, I realized that this argument wasn't used... so I dropped it. IIRC I initially added it because I was not sure if it would be needed. But since it is currently not used, better not have it. Adding it later won't be a major refactor. > [typos-lsp](https://github.com/tekumara/typos-lsp) found a `pasword` typo. That exists also in `services/auth/method` but that's for another time 😄

limiting-factor commented 2026年06月14日 08:02:34 +02:00

@oliverpool wrote in #3 (comment):

• Maybe a unittest.ClearFixtureTables() method would be a good investment

I'll try to keep that in mind for another day 🙏

@oliverpool wrote in https://codeberg.org/limiting-factor/forgejo/pulls/3#issue-5800472: > * Maybe a `unittest.ClearFixtureTables()` method would be a good investment I'll try to keep that in mind for another day 🙏