Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Add a Roslyn analyzer for global classes #79007

Merged

Conversation

398utubzyt
Copy link
Contributor

Should help clarify things to those who aren't as familiar with the core engine. This helps prevent one of many likely scenarios of the following from successfully compiling but not showing up in the editor (or even causing future crashes):

using Godot;

[GlobalClass]
public partial class ExampleGlobalGenericBad<T> : ExampleGlobalNode
{
    public override void _Ready()
    {
        GD.Print($"Woohoo, {GetType()}");
    }
}

image

@398utubzyt 398utubzyt requested a review from a team as a code owner July 4, 2023 03:31
@raulsntos raulsntos added this to the 4.2 milestone Jul 4, 2023
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing to the .NET module! This is looking great, I think it can be very helpful to have an analyzer for this.

Adding errors where there weren't before breaks source compat. But the analyzer adds errors to code that used to be invalid or otherwise would not work as expected, so I think we're OK with the breaking change.

Also, since you are a new contributor, make sure to read CONTRIBUTING.md and the contributing documentation if you haven't already.

You'll need to squash the commits before this PR can be merged. The contributing documentation contains information about squashing in case you need it.

Feel free to reach out in the development chat if you need help.

@398utubzyt 398utubzyt requested a review from raulsntos July 5, 2023 00:58
@398utubzyt 398utubzyt requested a review from raulsntos July 5, 2023 01:55
@YuriSizov YuriSizov changed the title C#: Roslyn Analyzer for Global Classes C#: Add a Roslyn analyzer for global classes Jul 5, 2023
@398utubzyt 398utubzyt force-pushed the dotnet/globalclass-analyzer branch from c86c600 to 2115721 Compare July 5, 2023 22:42
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This already looks great. I only added a few comments about the diagnostic messages to try and be consistent with existing diagnostic messages.

@398utubzyt 398utubzyt force-pushed the dotnet/globalclass-analyzer branch from 7a6754d to be49715 Compare July 7, 2023 21:59
@398utubzyt 398utubzyt requested a review from raulsntos July 7, 2023 22:00
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you very much!

Co-Authored-By: Raul Santos <raulsntos@gmail.com>
@398utubzyt 398utubzyt force-pushed the dotnet/globalclass-analyzer branch from 0e1bec2 to 8e56c80 Compare July 7, 2023 23:37
@YuriSizov YuriSizov merged commit bb6879e into godotengine:master Jul 14, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 14, 2023

Thanks! And congrats on your first merged Godot PR, I believe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants