karaoke-dev / karaoke

Will be the best karaoke system.
http://blog.karaoke.dev
GNU General Public License v3.0
207 stars 16 forks source link

Enable NRT by default #1390

Closed andy840119 closed 1 year ago

andy840119 commented 2 years ago

See how this issue did: https://github.com/ppy/osu/pull/18743/commits

We need to add this line in the .csproj, see this commit https://github.com/ppy/osu/pull/18743/commits/968640c95176f4a771ab6208234045f40e4ca281

<Nullable>enable</Nullable>

Also, notice that we will not add the #nullable in the .cs file. should fix all relative issue in the class.

And because it will be hard to fix it in a single PR, so might be separate into several parts:

In the test case:

And notice that should apply in the test project also. Highlight part means it's non-graphic and should be able to remove the #nullable

Note:

#!/bin/bash

for file in $( find "$@" -type f -name '*.cs' ); do
    # remove the nullable disable line generate in another script
    # install the brew install gnu-sed and use gsed instead of the sed commend.

    # see: https://unix.stackexchange.com/a/93909
    gsed -i 'N;/\n#nullable disable/d' "$file"
done

about the removing script

andy840119 commented 2 years ago

Will trying to import in the https://github.com/karaoke-dev/osu-framework-font/issues/222 first.

andy840119 commented 2 years ago

And note that https://github.com/karaoke-dev/LrcParser is using this flag as default.

see: https://github.com/karaoke-dev/LrcParser/blob/main/LrcParser/LrcParser.csproj

smoogipoo commented 2 years ago

See https://github.com/ppy/osu-framework/pull/5241 for an automated script

andy840119 commented 2 years ago

image Seems some of the compile issue is related to the ResolvedAttribute. Guess should wait until this issue solved.

Not really sure should mark this property as nullable because it will not be null after BDL.

andy840119 commented 2 years ago

Or will use null! in o!f and lazer?

[Resolved]
private ILyricRubyTagsChangeHandler rubyTagsChangeHandler { get; set; } = null!;
smoogipoo commented 2 years ago

Or will use null! in o!f and lazer?

[Resolved]
private ILyricRubyTagsChangeHandler rubyTagsChangeHandler { get; set; } = null!;

Yep, this is the only workaround we have (for now? for ever?).

andy840119 commented 2 years ago

image https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis Not have deep looking about this but seems AllowNullAttribute works also. Not really sure how to combine those two attributes, trying to solve this but got no answer.

andy840119 commented 2 years ago

@smoogipoo A quick question: Will Children or InternalChildren still accept null? Like this one:

public IReadOnlyList<T>? Children
{
    get
    {
        if (Content != this)
            return Content.Children;

        return internalChildrenAsT;
    }
    set => ChildrenEnumerable = value;
}

Or will change to this one which only accept empty array?

public IReadOnlyList<T> Children
{
    get
    {
        return internalChildrenAsT;
    }
    set => ChildrenEnumerable = value;
}
smoogipoo commented 2 years ago

Will eventually be non-null only, the second one above.

andy840119 commented 1 year ago

Man, this table did not update for a while.

andy840119 commented 1 year ago

image I think it's OK to close this issue because only has few nullable disable annotation in this project.