Skip to content

Conversation

@raulsntos
Copy link
Member

@raulsntos raulsntos commented Aug 6, 2023

Adds documentation pages for every C# diagnostic reported by Godot analyzers so they can be linked from the IDE.

These pages attempt to follow the same format as Microsoft's .NET documentation (example: CA1001).

@raulsntos raulsntos added enhancement topic:dotnet content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features labels Aug 6, 2023
@raulsntos raulsntos requested a review from a team August 6, 2023 16:38
To fix a violation of this rule, add the ``partial`` keyword to the type
declaration.

.. tip::
Copy link
Member

Choose a reason for hiding this comment

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

IMO this is not really helpful for this error as it you are going to have the boilerplate code collection of the century because you write all that generated code manually (so unlikely to ever happen) or you implement your own source generator (vastly more likely) and still have to mark that member as partial.

Is this already documented elsewhere or should this be moved to another page.

Copy link
Member Author

Choose a reason for hiding this comment

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

I consider disabling source generators to be an advanced use case, so I agree we could remove the tip or maybe make it less prominent. I just though it would make sense to mention all the ways in which you can "fix" the error.

Is this already documented elsewhere

It's not documented anywhere else, maybe it should be, but I don't really want to promote it too much since it's likely not what most users would want to do.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like this belongs on a page Pointers for advanced usage scenarios, but you have to go source diving to really realize them or alternatively it could just stay a thing that is mentions on a few Github issues, but otherwise can only be found in the source. tho mentioning it on the docs would make it clear that this is not just an internal thing for development.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to remove the tip and we can decide how to surface this feature in the future.

@@ -0,0 +1,74 @@
GD0002: Missing partial modifier on declaration of type which contains one or more subclasses of GodotObject
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GD0002: Missing partial modifier on declaration of type which contains one or more subclasses of GodotObject
GD0002: Missing partial modifier on declaration of type which contains one or more subclass of GodotObject

(I think) But this is probably because the actual diagnostic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I just copied the diagnostic title, we could change it. I think I would use nested classes rather than subclasses here, what do you think? But I think it has to be plural because of the one or more.

Copy link
Member

Choose a reason for hiding this comment

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

It just looked wrong to me, with some more thought, both versions are probably fine, but if we want to change it I would also go to a different wording like contains nested classes that are derived from GodotObject

Copy link
Member Author

@raulsntos raulsntos Aug 10, 2023

Choose a reason for hiding this comment

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

I like that wording. This change needs to be done in the main repository as well, is there any other diagnostic that you think could be worded better? It'd be nice to make all the changes in a single PR while we're at it.

WIP: raulsntos/godot@79622bb

Copy link
Member Author

Choose a reason for hiding this comment

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

@mhilbrunner mhilbrunner merged commit e5c7e63 into godotengine:master Aug 13, 2023
@mhilbrunner
Copy link
Member

Merged.

@raulsntos raulsntos deleted the dotnet/diagnostics branch August 13, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features enhancement topic:dotnet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants