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

Remove word duplicates in comments and strings, and fix casing and punctuation #88733

Merged

Conversation

ryevdokimov
Copy link
Contributor

Used regex to find these. Ignored third party files.

Fixes: #88711

@ryevdokimov ryevdokimov requested review from a team as code owners February 23, 2024 19:54
@KoBeWi KoBeWi added this to the 4.3 milestone Feb 23, 2024
@akien-mga
Copy link
Member

What regex did you use? Might be handy to re-run in the future as such issues slip through review.

@ryevdokimov
Copy link
Contributor Author

ryevdokimov commented Feb 23, 2024

Hold off on this, I found more with a refined search.

What regex did you use? Might be handy to re-run in the future as such issues slip through review.

Currently:

For comments: //.*\b(\w+)\b\s+\1\b (There are some results I ignored like owner owner, because I didn't know if it meant owner's owner, I didn't want to get too nitpicky)

For strings: (["'])(?:(?=(\?))\2.)?\b(\w+)\b(?:\s+\3\b)+(?:(?=(\?))\4.)?\1

Error messages: MSG(["'][a-z].*["'])

@ryevdokimov ryevdokimov force-pushed the Remove-word-duplicates branch from 1dcafea to e7c832f Compare February 23, 2024 20:39
@@ -661,7 +661,7 @@ class VariantConstructNoArgsNil {
VariantInternal::clear(r_ret);
}
static void ptr_construct(void *base, const void **p_args) {
ERR_FAIL_MSG("can't ptrcall nil constructor");
ERR_FAIL_MSG("Cannot ptrcall nil constructor");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regex for MSG( starting with lower case, updated a few to fit other messages I found.

Copy link
Member

Choose a reason for hiding this comment

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

While at it, error messages should also end with a dot.

Copy link
Contributor Author

@ryevdokimov ryevdokimov Feb 23, 2024

Choose a reason for hiding this comment

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

What about exclamation points? Should these become "." to be consistent? To me all errors are equally exciting.

https://github.com/godotengine/godot/blob/da4f50851d15c073ca413196aa1501fb97108c07/scene/resources/skeleton_modification_2d_twoboneik.cpp#L286

Copy link
Member

Choose a reason for hiding this comment

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

In most cases yeah, the exclamation mark might be overdoing it a bit. But that might imply further improving the phrasing for consistency with other similar errors, which may be outside the scope of this PR :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Leaving them alone for now.

joint_one_bone2d_node_cache = node->get_instance_id();

Bone2D *bone = Object::cast_to<Bone2D>(node);
if (bone) {
joint_one_bone_idx = bone->get_index_in_skeleton();
} else {
ERR_FAIL_MSG("update joint one Bone2D cache: Nodepath to Bone2D is not a Bone2D node!");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lower case to upper case.

@ryevdokimov ryevdokimov changed the title Remove word duplicates in comments and strings Remove word duplicates in comments and strings, and fix casing Feb 23, 2024
@ryevdokimov ryevdokimov force-pushed the Remove-word-duplicates branch 2 times, most recently from da4f508 to bd8ea9b Compare February 23, 2024 21:03
@ryevdokimov ryevdokimov requested a review from a team as a code owner February 23, 2024 21:03
@ryevdokimov ryevdokimov changed the title Remove word duplicates in comments and strings, and fix casing Remove word duplicates in comments and strings, and fix casing and punctuation Feb 23, 2024
Comment on lines 119 to 122
ERR_FAIL_MSG("Cannot remove file or directory: " + p_path + ".");
}
} else {
ERR_FAIL_MSG("Cannot remove non-existent file or directory: " + p_path);
ERR_FAIL_MSG("Cannot remove non-existent file or directory: " + p_path + ".");
Copy link
Member

@akien-mga akien-mga Feb 23, 2024

Choose a reason for hiding this comment

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

I think in this case, the omission was intentional, to avoid confusion at to whether the dot is part of the file path or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, yeah that makes sense. I should avoid doing it to file paths. Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are existing examples of adding the period at the end of the path. Would you like me to remove those?

ERR_FAIL_V_MSG(ERR_FILE_UNRECOGNIZED, "Unknown config file format: " + p_path + ".");

Copy link
Member

Choose a reason for hiding this comment

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

What's most commonly used currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the dot is used 3x as much.

image

vs

image

Copy link
Member

Choose a reason for hiding this comment

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

Let's standardize on no dots for these then.

core/math/basis.cpp Outdated Show resolved Hide resolved
@ryevdokimov ryevdokimov force-pushed the Remove-word-duplicates branch 2 times, most recently from 610e331 to dd623b7 Compare February 23, 2024 22:06
@ryevdokimov ryevdokimov force-pushed the Remove-word-duplicates branch from dd623b7 to a205717 Compare February 23, 2024 22:21
@@ -1860,7 +1860,7 @@ void RenderingDeviceDriverD3D12::command_pipeline_barrier(
VectorView<RDD::BufferBarrier> p_buffer_barriers,
VectorView<RDD::TextureBarrier> p_texture_barriers) {
if (p_src_stages.has_flag(PIPELINE_STAGE_ALL_COMMANDS_BIT) && p_dst_stages.has_flag(PIPELINE_STAGE_ALL_COMMANDS_BIT)) {
// Looks like the intent is a a full barrier.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased to fix recent merge conflict and found another one in the process.

@ryevdokimov ryevdokimov force-pushed the Remove-word-duplicates branch from a205717 to 13e8209 Compare February 23, 2024 22:28
@@ -1597,7 +1597,7 @@ void ED_SHORTCUT_OVERRIDE(const String &p_path, const String &p_feature, Key p_k
ERR_FAIL_NULL_MSG(EditorSettings::get_singleton(), "EditorSettings not instantiated yet.");

Ref<Shortcut> sc = EditorSettings::get_singleton()->get_shortcut(p_path);
ERR_FAIL_COND_MSG(!sc.is_valid(), "Used ED_SHORTCUT_OVERRIDE with invalid shortcut: " + p_path + ".");
ERR_FAIL_COND_MSG(!sc.is_valid(), "Used ED_SHORTCUT_OVERRIDE with invalid shortcut: " + p_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand, are error messages with paths meant to end with, or without punctuation? What's the rule here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is without: #88733 (comment)

@akien-mga akien-mga merged commit 83b32f9 into godotengine:master Feb 25, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@ryevdokimov ryevdokimov deleted the Remove-word-duplicates branch February 26, 2024 23:11
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.

Typo in message (word duplicated)
4 participants