search

ppy / osu-framework

A game framework written with osu! in mind.
MIT License
1.65k stars 416 forks source link

Resolved attribute can set "CanBeNull" as a param or field #3849

Open Joehuu opened 4 years ago

Joehuu commented 4 years ago

In the osu repository, I see cases of [Resolved(canBeNull: true)] (35) and [Resolved(CanBeNull = true)] (50). The framework sets with only the field. Just a code quality issue and discussion to make the field private (which it can) or remove param.

ghost commented 4 years ago

Same can be said with CachedAttribute.

I would choose to make the field private as the attribute isn't really meant to be modified and its properties shall be set once at construction only.

The properties can become obsolete until 6 months before removal to avoid sudden breaking changes.

YVbakker commented 3 years ago

Same can be said with CachedAttribute.

Which attribute(s) do you specifically mean there?

I would choose to make the field private as the attribute isn't really meant to be modified and its properties shall be set once at construction only.

I agree, it would be an invitation for misuse if one could decide halfway through the code to allow the CachedAttribute to be null or vice versa.

The properties can become obsolete until 6 months before removal to avoid sudden breaking changes.

I found a wiki page describing breaking changes. As this is one (albeit a small one), should it be a TODO to document this there as well?

frenzibyte commented 3 years ago

Same can be said with CachedAttribute.

Which attribute(s) do you specifically mean there?

The issue was originally about ResolvedAttribute only, but CachedAttribute also has the same issue.

I would choose to make the field private as the attribute isn't really meant to be modified and its properties shall be set once at construction only. I agree, it would be an invitation for misuse if one could decide halfway through the code to allow the CachedAttribute to be null or vice versa.

They should not actually be private, but instead public readonly.

The properties can become obsolete until 6 months before removal to avoid sudden breaking changes.

I found a wiki page describing breaking changes. As this is one (albeit a small one), should it be a TODO to document this there as well?

In the case of fields, it is not possible to obsolete the setting part of them, you can only just change them to become readonly.

But in general, breaking changes get written before the PR is merged, so there won't be any reason of having a TODO in code for such.

YVbakker commented 3 years ago

Ah you beat me to it, sorry for wasting your time .-. and thanks for the clarification

I was way overthinking this after reading into the project a bit (and the first paragraph of this issue). I'll update the PR to reflect the changes.

smoogipoo commented 3 years ago

Before anyone else attempts this, please first provide reasonings for wanting to make a change such as what the C# best practices say to do or what is commonly accepted by the wider C# community.