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

Clarify Array class methods that return error #80936

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

curious-broccoli
Copy link
Contributor

insert now returns an Error value so the note had to be changed. only needs to apply for 4.x I believe

perhaps the note can be removed altogether

@curious-broccoli curious-broccoli requested a review from a team as a code owner August 23, 2023 16:32
@curious-broccoli
Copy link
Contributor Author

In my opinion the description for insert and resize should mention that the return value is an Error. This probably applies to other functions as well

@AThousandShips
Copy link
Member

The specification isn't needed IMO, "a value" and "an array" are equally true and we want to keep changes to the docks for non-critical reasons to a minimum

@curious-broccoli
Copy link
Contributor Author

Well, I think it's bad that it says that nothing/no value is returned when actually an integer is returned. If the note was removed and instead the description said that an error (or an integer equaling an error) is returned it would also be clear.

@AThousandShips
Copy link
Member

AThousandShips commented Aug 23, 2023

My bad that is true, then it should be changed to indicate that a error value is returned

@YuriSizov YuriSizov changed the title Fix Array class docs after #47406 Mention Array class methods returning Error Aug 24, 2023
doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Array.xml Show resolved Hide resolved
doc/classes/Array.xml Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

Would be good to look into why the Variant bindings here do not carry over the enum information, it should be documented as Error.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

LGTM now!

@YuriSizov YuriSizov changed the title Mention Array class methods returning Error Clarify Array class methods that return error Aug 24, 2023
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.

Other than that nitpick LGTM

@curious-broccoli
Copy link
Contributor Author

Should those array functions that return a int/Error show the return value as Error in the docs like it is here

<return type="int" enum="Error" />
?

@AThousandShips
Copy link
Member

It should but it can't, see my comment above

@curious-broccoli
Copy link
Contributor Author

It should but it can't, see my comment above

well, for Array it is only <return type="int" /> while for ConfigFile it is <return type="int" enum="Error" />. As someone who hardly knows how to write the docs I'd assume that is the problem 🙈

@AThousandShips
Copy link
Member

The problem is that the generation system for documentation doesn't detect metadata and decorations for arguments and return values, I am aware of the difference, that's why I pointed this issue out, it's related to low level code

@YuriSizov
Copy link
Contributor

Even with the return type fixed, it never hurts to be explicit in the description about what the method takes and returns. So for now fixing the docs would be fine.

curious-broccoli added a commit to curious-broccoli/godot that referenced this pull request Aug 24, 2023
@YuriSizov YuriSizov merged commit e538843 into godotengine:master Aug 25, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks! And congrats on your first merged Godot PR!

@AThousandShips AThousandShips added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 17, 2023
@AThousandShips
Copy link
Member

Marked for cherry picking as I believe the doc changes apply to 4.1 as well and clarify things

mandryskowski pushed a commit to mandryskowski/godot that referenced this pull request Oct 11, 2023
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

I think we can skip cherry-picking this one.

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.

4 participants