getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.23k stars 166 forks source link

More type hints #6494

Closed distantnative closed 1 week ago

distantnative commented 2 weeks ago

Description

Summary of changes

Reasoning

Keeps our code safer, also our tests, by hardcoding our implicit assumptions.

Additional context

I know these PRs are always huge and thus hard to review. I don't think breaking them down into separate PRs would help much here. The changes are simple, it's just a lot of them. So having dozens of PRs instead, doesn't really help to reduce the review load.

We could contemplate if we are ok with merging these kind of PRs during alpha without full review (similarly how we push some changes already directly), as the alpha and RC phase in combination with existing unit tests could be a sufficient baseline. And the cost-benefit ratio of a full review might be not that great compared to merging without full review and occasionally running into a small regression we can fix then.

Changelog

Breaking changes

Ready?

For review team

distantnative commented 1 week ago

I explicitly stayed away from content classes for this reason.

distantnative commented 1 week ago

@lukasbestle i think your comment/suggestion makes a lot of sense. And as you say it's more a matter of finding a good way to split these up again. Do you have a good way for keeping track of such todo notes?

lukasbestle commented 1 week ago

I just use a blank text document in TextEdit. It's just temporary anyway. Or if it's about a specific line, I add a TODO comment that I will then see when going through the changes in Tower to select them for staging. Basically skipping over that TODO comment then and first committing everything else.