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

Allow splitting GodotDisabledSourceGenerators by comma (Updates ExtensionMethods.cs) #88682

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

InfiniteCactusDev
Copy link

@InfiniteCactusDev InfiniteCactusDev commented Feb 22, 2024

Allows separating GodotDisabledSourceGenerators by comma

There's a bug in the Roslyn analyzers that describes the ; character being treated as a comment in the resulting editorconfig file and thus stripping all values after it: dotnet/roslyn#43970

I noticed this when trying to disable multiple source generators in my csproj file like so:

<GodotDisabledSourceGenerators>ScriptSignals;ScriptProperties</GodotDisabledSourceGenerators>

This results in the following line in *.GeneratedMSBuildEditorConfig.editorconfig:

    build_property.GodotDisabledSourceGenerators = ScriptSignals;ScriptProperties

Which results in only ScriptSignals to be disabled. ScriptProperties is ignored and doesn;t reach the checking-code.


This change allows to also split the setting with a comma.

Allow separating GodotDisabledSourceGenerators by comma
@InfiniteCactusDev InfiniteCactusDev requested a review from a team as a code owner February 22, 2024 20:21
@InfiniteCactusDev InfiniteCactusDev changed the title Update ExtensionMethods.cs Allow splitting GodotDisabledSourceGenerators by comma (Updates ExtensionMethods.cs) Feb 22, 2024
@paulloz
Copy link
Member

paulloz commented Feb 22, 2024

Hello, and thank you for the contribution! ✨

The issue is indeed easily reproducible (just added unit tests for it). It looks like there was some recent movement on dotnet/roslyn#51625. If the issue is being fixed upstream, I would rather not introduce an ad-hoc behaviour on our end as a workaround. But I agree that it is currently problematic. Would love to get the insights of other contributors.

@InfiniteCactusDev
Copy link
Author

The reason behind the comma is that it's the next best logic separator to use and it's backwards compatible (there's no generator with a comma in the name).

@paulloz paulloz modified the milestones: 4.3, 4.x Apr 19, 2024
@Panthr75
Copy link

This PR fixes #91999

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