Skip to content

N28: godot-rust #854

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

Merged
merged 3 commits into from
Dec 5, 2021
Merged

N28: godot-rust #854

merged 3 commits into from
Dec 5, 2021

Conversation

Bromeon
Copy link
Contributor

@Bromeon Bromeon commented Dec 1, 2021

Part of #852

@AngelOnFira
Copy link
Member

I'll have to take another look at this one in a bit, it has two images so we'll likely have to find a way to make it one.

@AngelOnFira
Copy link
Member

I think this might be a bit long. @Bromeon I'm going to make a commit with my suggested changes, then I'll get you to look it over :)

image

@AngelOnFira
Copy link
Member

The paragraphs at the end are still long, but I'm inclined to let it slide for this section. @ozkriff @17cupsofcoffee what are your thoughts?

image

Copy link
Collaborator

@17cupsofcoffee 17cupsofcoffee left a comment

Choose a reason for hiding this comment

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

The length is a bit much, but I'm happy to go ahead with this.

If we want to shorten it a bit more, we could drop the list of changed symbols? People can click through if they want to know what specifically has changed, so it's not essential to list here.

Comment on lines 89 to 90
The improvements are best expressed as a picture -- these are differences
between version v0.9.3 and now:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line doesn't make sense now that the picture is at the top - I think it'd read fine if we removed it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to

Some differences between v0.9.3 and now can be seen in the above picture.

Copy link
Contributor Author

@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 for the review!

I shortened the text a bit:

  • Removed symbol list, just mention 2 of 4 examples inline in the text itself
  • Structured 3 refactoring changes as bullet list, making it a bit more compact
  • Removed line:

    Smaller changes include safety bugfixes or a GodotString::format() method

I rebased the changes onto latest source and fixed conflicts. Previous state would be bb4b86a.

Comment on lines 89 to 90
The improvements are best expressed as a picture -- these are differences
between version v0.9.3 and now:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to

Some differences between v0.9.3 and now can be seen in the above picture.

Copy link
Member

@AngelOnFira AngelOnFira left a comment

Choose a reason for hiding this comment

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

Thanks for the revision! Looks good to me now :)

@AngelOnFira AngelOnFira merged commit f575872 into rust-gamedev:source Dec 5, 2021
@Bromeon
Copy link
Contributor Author

Bromeon commented Dec 5, 2021

Thank you! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants