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

Implement Display and Debug for Builtins #553

Merged
merged 1 commit into from
Jan 6, 2024

Conversation

0awful
Copy link
Contributor

@0awful 0awful commented Jan 4, 2024

Closes #110.

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-553

@0awful 0awful force-pushed the display-and-debug branch 2 times, most recently from ff2a6a8 to 0b38ae6 Compare January 5, 2024 00:06
@0awful
Copy link
Contributor Author

0awful commented Jan 5, 2024

Somehow this picked up a bunch of other people's changes. I must've messed up the git rebase -i git push -f origin <branch-name>.

@0awful 0awful changed the title debug-display-for-buildins Implement Display and Debug for Builtins Jan 5, 2024
@0awful
Copy link
Contributor Author

0awful commented Jan 5, 2024

Somehow this picked up a bunch of other people's changes. I must've messed up the git rebase -i git push -f origin <branch-name>.

Cleaned this up, but now have a different commit from merging in master

@0awful 0awful force-pushed the display-and-debug branch from a645ca3 to 1808118 Compare January 5, 2024 04:28
@0awful
Copy link
Contributor Author

0awful commented Jan 5, 2024

Got it working. 👍

@0awful 0awful marked this pull request as ready for review January 5, 2024 04:30
@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Jan 5, 2024
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 🙂

Small tip, if you change your initial post to write "Closes #110.", then GitHub will recognize the relation and automatically close the issue upon merging.

Comment on lines 681 to 685
/// Example:
/// ```no_run
/// # use godot::prelude::*;
/// let array = array![1,2,3,4];
/// assert_eq!(format!("{}", array), "[1, 2, 3, 4]");
Copy link
Member

Choose a reason for hiding this comment

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

This misses closing code tags. "Example" could also be a section title like elsewhere.
I also made things a bit more compact and less ambiguous with "array" identifiers:

Suggested change
/// Example:
/// ```no_run
/// # use godot::prelude::*;
/// let array = array![1,2,3,4];
/// assert_eq!(format!("{}", array), "[1, 2, 3, 4]");
/// # Example
/// ```no_run
/// # use godot::prelude::*;
/// let a = array![1, 2, 3, 4];
/// assert_eq!(format!("{a}"), "[1, 2, 3, 4]");
/// ```

if count != 0 {
write!(f, ", ")?;
}
write!(f, "{}", v)?;
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
write!(f, "{}", v)?;
write!(f, "{v}")?;

Copy link
Member

Choose a reason for hiding this comment

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

Btw: is this ChatGPT 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.

Not at all 😂. Just followed the patterns of rust by example and the std library docs.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then ChatGPT stole it from there 😅

@@ -675,6 +675,26 @@ impl<T: GodotType> fmt::Debug for Array<T> {
}
}

impl<T: GodotType + fmt::Display> fmt::Display for Array<T> {
/// Formats `Array` to match Godot's string representation
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
/// Formats `Array` to match Godot's string representation
/// Formats `Array` to match Godot's string representation.

(We should possibly change this elsewhere too, but we adopted this convention of full sentences only after some time).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating all instances in my comments to have the full sentences paradigm.

Comment on lines 455 to 462
#[itest]
fn array_should_format_with_display() {
assert_eq!("[1, 2, 3, 4]", format!("{}", array![1, 2, 3, 4]));
assert_eq!("[]", format!("{}", Array::<real>::new()));
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you use assert_eq!(actual, expected) convention like existing tests? I.e. swap arguments.

Also don't hesitate to create some "actual" variables for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

Comment on lines 619 to 630
#[itest]
fn dictionary_should_format_with_display() {
assert_eq!(format!("{}", Dictionary::new()), "{ }");
assert_eq!(
format!(
"{}",
dict! {
"one": 1,
"two": true,
"three": Variant::nil()
}
),
"{ one: 1, two: true, three: <null> }"
)
}
Copy link
Member

Choose a reason for hiding this comment

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

Also this would benefit from extracting expressions into variables. Ideally the assert statements are single-line and not nested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will Do.

Comment on lines 311 to 323
impl fmt::Display for Dictionary {
/// Formats `Dictionary` to match Godot's string representation
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{{ ")?;
for (count, entry) in self.iter_shared().enumerate() {
if count != 0 {
write!(f, ", ")?;
}
write!(f, "{}: {}", entry.0, entry.1)?;
}
write!(f, " }}")
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Are the spaces after { and before } consistent with the way Godot formats dictionaries?

Copy link
Contributor Author

@0awful 0awful Jan 5, 2024

Choose a reason for hiding this comment

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

Yes, Its quite strange but it is how an all dictionaries are logged out. Empty -> { } and others have spaces between the braces and the key/value pairs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Particularly strange because arrays don't do that.

@0awful 0awful force-pushed the display-and-debug branch from d7f2abc to b5fa2e3 Compare January 5, 2024 21:14
@0awful
Copy link
Contributor Author

0awful commented Jan 5, 2024

Added Closes #110 and made changes called out by @Bromeon

  1. Switched ordering of assert_eq!() to always use (actual, expected) semantics
  2. Broke out the setup for test cases to allow for 1 line assert_eq!() statements
  3. Changed write!() macro invocations to take advantage of named variables ("{a}") over ("{}", a)
  4. Added periods at the end of sentences
  5. Made sure the code block was closed

@Bromeon
Copy link
Member

Bromeon commented Jan 6, 2024

Thanks a lot!

@Bromeon Bromeon added this pull request to the merge queue Jan 6, 2024
Merged via the queue into godot-rust:master with commit 68801cb Jan 6, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Debug + Display for builtins
3 participants