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

Overhaul Dictionary Documentation #69594

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Dec 5, 2022

Continuation of especially #69451 and #68838...

  • Made the wording match how other classes are documented a lot more closely;

  • Made use of [param] and several other strong references more often;

  • Changed words where it felt necessary, where they could be mistaken for another concept of Godot's API, where technical terms are more appropriate. For example:

    • "key/value pair" -> "entry".
      • I've most commonly seen people referring to a Dictionary's element as an "entry". Discussions are welcome, please.
  • Modified examples extensively, with the main goal to improve readability and variety:

    • Removed quotes around results when they could be misunderstood as strings (# Prints "5" -> # Prints 5);
    • Improved consistency all around;
    • Added extra padding before comments.
    • Avoid overly long lines.
  • COMPLETELY stripped away the part about Dictionary comparisons and hashes from the leading description.

    • Way to give nausea to users at first sight. Not that Dictionaries themselves are compared THAT much anyhow. Sheesh...
  • Updated Dictionary(from) constructor's description to be correct.


Feedback is very, very welcome.

@Mickeon Mickeon requested a review from a team as a code owner December 5, 2022 08:47
if "Godot" in {"Godot": 4}:
print("The key is here!") # Will be printed.
[/codeblock]
[b]Note:[/b] This method returns [code]true[/code] as long as the [param key] exists, even if its corresponding value is [code]null[/code].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This small note may or may not be mergeable with the very first sentence. Something along the lines of "Returns [code]true[/code] if the dictionary contains an entry with the given [param key], regardless of its corresponding value"

@Mickeon Mickeon force-pushed the doc-peeves-read-a-dictionary branch 2 times, most recently from 38f7853 to aebd72c Compare December 5, 2022 08:56
@Calinou Calinou added this to the 4.0 milestone Dec 5, 2022
Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

Changes look good, spotted nothing so far. Will let it stay open for a bit longer and merge if not one else notices anything.

@Mickeon
Copy link
Contributor Author

Mickeon commented Dec 5, 2022

I should've left it as a draft as I also wanted to revamp the leading description. It's better to merge it ASAP though I suppose.

@Mickeon Mickeon force-pushed the doc-peeves-read-a-dictionary branch 2 times, most recently from bba2f73 to 3416281 Compare December 5, 2022 15:10
@Mickeon
Copy link
Contributor Author

Mickeon commented Dec 5, 2022

Updated. Also added a small instruction on how to iterate a dictionary in the leading description, because if I didn't I would've later felt guilty about it.

@Mickeon Mickeon force-pushed the doc-peeves-read-a-dictionary branch from 3416281 to d68df24 Compare December 5, 2022 15:30
@Mickeon
Copy link
Contributor Author

Mickeon commented Dec 5, 2022

Thank you very much for the feedback

@Mickeon Mickeon requested a review from raulsntos December 5, 2022 15:31
@Mickeon Mickeon force-pushed the doc-peeves-read-a-dictionary branch from d68df24 to b77f935 Compare December 5, 2022 16:18
@akien-mga akien-mga merged commit 65cbcae into godotengine:master Dec 5, 2022
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the doc-peeves-read-a-dictionary branch December 5, 2022 17:33
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.

5 participants