Skip to content

Enforce p_ prefixes for arguments in binds#1490

Merged
dsnopek merged 1 commit intogodotengine:masterfrom
AThousandShips:arg_name_fix
Jun 18, 2024
Merged

Enforce p_ prefixes for arguments in binds#1490
dsnopek merged 1 commit intogodotengine:masterfrom
AThousandShips:arg_name_fix

Conversation

@AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jun 14, 2024 •
edited
Loading

Improves general readability of generated code and prevents potential collisions

@AThousandShips AThousandShips added bug This has been identified as a bug topic:buildsystem Related to the buildsystem or CI setup cherrypick:4.1 cherrypick:4.2 labels Jun 14, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Jun 14, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner June 14, 2024 10:51
@AThousandShips AThousandShips mentioned this pull request Jun 14, 2024
Merged
AThousandShips
AThousandShips commented Jun 14, 2024
View reviewed changes
@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 14, 2024 •
edited
Loading

My bad, missed this:

Just making this an improvement

@AThousandShips AThousandShips added enhancement This is an enhancement on the current functionality and removed bug This has been identified as a bug cherrypick:4.1 cherrypick:4.2 labels Jun 14, 2024
@AThousandShips AThousandShips modified the milestones: 4.3, 4.x Jun 14, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Jun 14, 2024

Thanks!

This makes sense to me: we use the p_ prefix in all the hand-written code, so let's do it for all the generated code too.

The changes here look good in a quick skimming! This just needs a rebase after having merged PR #1485

@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 14, 2024 •
edited
Loading

Some of the cases like Array has some non-prefixed cases, should I fix all the generated code with this? Like String etc. as well?

Edit: Since these are actually generated with these arguments in the source with this change I'll fix it all

@AThousandShips AThousandShips force-pushed the arg_name_fix branch 2 times, most recently from ea2156d to 0b703c3 Compare June 14, 2024 13:29
@dsnopek
Copy link
Collaborator

dsnopek commented Jun 14, 2024

Some of the cases like Array has some non-prefixed cases, should I fix all the generated code with this? Like String etc. as well?

I think we ultimately want to change all cases, so if there aren't any roadblocks to doing it now, let's do it!

@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 14, 2024

Realized a few binds include r_ so will make those not include the p_, will push

@AThousandShips AThousandShips mentioned this pull request Jun 14, 2024
Merged
dsnopek
dsnopek approved these changes Jun 17, 2024
View reviewed changes
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great to me :-)

@dsnopek dsnopek merged commit ed1e963 into godotengine:master Jun 18, 2024
@AThousandShips AThousandShips deleted the arg_name_fix branch June 18, 2024 15:44
@dsnopek
Copy link
Collaborator

dsnopek commented Jul 17, 2024

Cherry-picked for 4.2 in PR #1527

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 17, 2024

Cherry-picked for 4.1 in PR #1529

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsnopek dsnopek dsnopek approved these changes

Assignees

No one assigned

Labels

enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

3 participants

@AThousandShips @dsnopek @akien-mga