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

Documentation: Add support for deprecated/experimental messages #81458

Merged

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Sep 8, 2023

Add support for deprecated/experimental messages to core and GDScript. See:

## my_func desc.
## @deprecated: Use [method another] instead.
func my_func():
    pass



@YuriSizov
Copy link
Contributor

This would be a breaking change for the XML format. We should find a way to avoid that.

@dalexeev
Copy link
Member Author

dalexeev commented Sep 8, 2023

We can use two attributes, bool and string, just like in DocData structures. But it's less compact. Or we can add compatibility code for the old boolean attributes, which will allow the old XMLs to be read, but on write they will be saved in the new format. It's funny if we deprecate the is_deprecated attribute.

@dalexeev dalexeev force-pushed the doc-add-deprected-experimental-message branch from 962a286 to 402f0a8 Compare September 10, 2023 11:12
@raulsntos
Copy link
Member

I love this, we've been generating generic messages like This method is deprecated in the generated C# API and now we'll be able to provide something more informative.

I noticed there were some empty deprecation messages. Should we try to avoid empty messages or is it intended that some messages will be empty? We can still provide the generic message in C# in these cases, but I think it'd be better to provide a better message. I'm not suggesting to fill all the messages in this PR, it could be a follow-up.

I'm not sure how useful it is for experimental API though, it seems all the messages are empty in this PR. I also don't know what we would write here other than a generic This API is experimental and may change in the future. We also wouldn't be able to use these messages in C#, the [Experimental] attribute doesn't allow providing a message, but that shouldn't block this if it's still useful.

modules/gltf/doc_classes/GLTFDocument.xml Show resolved Hide resolved
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8" ?>
<class name="SkeletonIK3D" inherits="Node" is_deprecated="true" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd">
<class name="SkeletonIK3D" inherits="Node" deprecated="" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd">
Copy link
Member

Choose a reason for hiding this comment

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

I think there was someone asking recently why this class was deprecated, worried that it will be removed without an alternative. Maybe we could add a message here that clarifies that a replacement will be provided in the future?

Copy link
Member

Choose a reason for hiding this comment

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

CC @godotengine/animation

@dalexeev
Copy link
Member Author

I noticed there were some empty deprecation messages. Should we try to avoid empty messages or is it intended that some messages will be empty?

I think so, but I've left the messages empty for now. We should also think about default messages. Now I've added a neutral "This ... may be changed or removed in future versions.", but this should probably be different for Deprecated and Experimental and for engine and user code.

I'm not sure how useful it is for experimental API though, it seems all the messages are empty in this PR. I also don't know what we would write here other than a generic This API is experimental and may change in the future.

Probably yes, although I think that sometimes we can indicate problems when using the experimental API:

@@ -273,8 +289,13 @@
</xs:sequence>
<xs:attribute type="xs:string" name="name" />
<xs:attribute type="xs:string" name="inherits" />
<!-- deprecated -->
<xs:attribute type="xs:float" name="version" use="optional" />
Copy link
Member Author

Choose a reason for hiding this comment

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

@dalexeev dalexeev force-pushed the doc-add-deprected-experimental-message branch from 402f0a8 to 4fde31e Compare September 10, 2023 16:32
@dalexeev dalexeev marked this pull request as ready for review September 10, 2023 16:34
@dalexeev dalexeev requested review from a team as code owners September 10, 2023 16:34
Comment on lines +1494 to +1598
deprecated_prefix = translate("Deprecated:")
if item.deprecated.strip() == "":
default_message = translate(f"This {item.definition_name} may be changed or removed in future versions.")
result += f"**{deprecated_prefix}** {default_message}\n\n"
else:
result += f"**{deprecated_prefix}** {format_text_block(item.deprecated.strip(), item, state)}\n\n"
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a way to use color in RST? This will probably require some changes to the site so that we can use:

:red:`Deprecated:"` Message.

or

.. deprecated:: Message.

Now it looks like this:

I only added this to the descriptions, not to the overviews.

Copy link
Member

@Calinou Calinou Sep 10, 2023

Choose a reason for hiding this comment

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

rST doesn't support arbitrary colors (aside of raw HTML tags). You could use the .. danger:: adominition though, which appears in red.

See also #63079.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM style and code wise

doc/classes/AnimationPlayer.xml Outdated Show resolved Hide resolved
doc/classes/AnimationTree.xml Outdated Show resolved Hide resolved
doc/classes/NavigationPolygon.xml Outdated Show resolved Hide resolved
@dalexeev dalexeev force-pushed the doc-add-deprected-experimental-message branch 2 times, most recently from cbd43ce to bf9b2a8 Compare February 12, 2024 21:25
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master f317cc7), it mostly works as expected.

I can't get the deprecated/experimental reason messages to show up though:

extends Node2D

## Description.
##@deprecated: Test reason
var var1

##@experimental: Test reason 2
var var2

# Test comment (not a docblock).
##@deprecated: Test reason 3
func one() -> void:
	pass


##@experimental: Test reason 4
##Description.
func two() -> void:
	pass

image

@dalexeev
Copy link
Member Author

@Calinou Looks like you were testing an older version:

@dalexeev dalexeev force-pushed the doc-add-deprected-experimental-message branch 2 times, most recently from 53d338a to eea19d6 Compare February 13, 2024 09:16
@dalexeev
Copy link
Member Author

dalexeev commented Feb 13, 2024

I have completed self-review and testing. This initially small PR grew into a significant EditorHelp refactoring:

  • Add missing XML escaping in DocTools.
  • Rename "Theme Item" to "Theme Property" in various make_rst.py and editor_help.cpp messages for consistency. This does not break compatibility, the [theme_item] tag is left unchanged.
  • Fix a bug with [theme_item] navigation for external classes.
  • Unify blank lines for all Editor Help sections.
  • Fix theme property anchors.
  • Replace find() with find_char() for optimization.
  • Improve grouping of push_*() and pop() calls, add comments for consistency.
  • Fix usage of text append methods:
    • class_desc->add_text() - Used to add text "as is".
    • class_desc->append_text() - Used to append text that is guaranteed to contain only standard BBCodes.
    • _add_text() - Used if the text can contain custom EditorHelp markup.
  • Add missing _fix_constant() and value color.
  • Improve detection of multiline descriptions for constants and enum elements.
  • Improve link for property overrides.

@dalexeev dalexeev force-pushed the doc-add-deprected-experimental-message branch from eea19d6 to 513a46f Compare February 13, 2024 11:04
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great to me overall!

doc/classes/@GlobalScope.xml Outdated Show resolved Hide resolved
doc/classes/@GlobalScope.xml Outdated Show resolved Hide resolved
doc/classes/@GlobalScope.xml Outdated Show resolved Hide resolved
doc/classes/AnimationPlayer.xml Outdated Show resolved Hide resolved
doc/classes/InputEventJoypadButton.xml Outdated Show resolved Hide resolved
doc/classes/MultiMesh.xml Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8" ?>
<class name="SkeletonIK3D" inherits="Node" is_deprecated="true" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd">
<class name="SkeletonIK3D" inherits="Node" deprecated="" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd">
Copy link
Member

Choose a reason for hiding this comment

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

CC @godotengine/animation

@akien-mga
Copy link
Member

We should probably find some consensus on how to documentation deprecated and experimental methods/properties, and apply it throughout the codebase (cc @godotengine/documentation).

Notably, how to document a method/property when it's deprecated because:

  • It doesn't do anything and we're just keeping it to avoid breaking API compat
  • It still does something but was replaced by a new method/property which should be referenced
  • It still does something but is no longer supported, and no alternative is given

We can now put information in the deprecated message, but there's still the main description to deal with. What should it contain? Should it be kept empty if all the relevant info is in the deprecation message? If so, should we change doc/tools/doc_status.py so it considers an empty description of a method/property with a deprecated message as fully documented?

@Mickeon
Copy link
Contributor

Mickeon commented Feb 15, 2024

I was taking a look. In my opinion, to make it extremely short:

  • The deprecated/experimental message should explain "why" and/or provide the better alternative(s);
  • The description should explain "what". Very, very briefly.
    • I can't think of exceptions. Not saying what something is/was feels wrong to me.

Also these messages are not translatable now, are they?

@dalexeev
Copy link
Member Author

Also these messages are not translatable now, are they?

Yes, after merging this PR, some changes are needed in extract_classes.py.

As for the docs, I agree with you, but I think that in any case we will need a follow-up PR for adjustments. However, I'm ready to apply suggestions that we are already confident about.

@Mickeon
Copy link
Contributor

Mickeon commented Feb 15, 2024

I don't mind scouting across the class reference a second time as some form of standardization, post-merge. I got nothing better to do...

@dalexeev dalexeev force-pushed the doc-add-deprected-experimental-message branch from 513a46f to af28f87 Compare February 15, 2024 12:59
@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 15, 2024
@akien-mga akien-mga merged commit be7229f into godotengine:master Feb 15, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

"Deprecated" needs to be more visible in the documentation
7 participants