search

ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
14.67k stars 2.17k forks source link

DrawableHitObject.HitObject should be marked as nullable #28355

Open jav76 opened 1 month ago

jav76 commented 1 month ago

Add the [CanBeNull] attribute to DrawableHitObject.HitObject and update xmldocs.

25347

bdach commented 1 month ago

Marking the property nullable I can sort of agree with in principle but I don't really agree with renaming it. It feels pointless to rename it at this stage and additionally we're still at the stage where pooling is still entirely optional and you still can instantiate a single DHO for a single object and completely ignore the existence of pooling, at which point using language like "current" hitobject stops making sense.

If you wanna mention pooling that mention can go live in xmldoc.

peppy commented 1 month ago

Agree that naming is fine. For other cases of pooling / drawable model representations we don't prefix Current. Adjusting xmldoc will work for this.

If you're going to change this PR to undo the naming change, please force push the name changes out of the commit for cleanness.

smoogipoo commented 1 month ago

Not sure why we would be leaning into attributes, as opposed to removing the #nullable disable and using NRT.

bdach commented 1 month ago

We can, but I presume that would balloon this diff from what should be probably something like 50 files (rather than one) into probably around 150 if you wanted to do it all in one fell swoop. Might also run into other things marked with jetbrains attributes that might need adjusting. Dunno, would have to go and try that myself.