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

Add override mark to ResourceFormat class #89863

Merged
1 commit merged into from
Mar 25, 2024

Conversation

ppphp
Copy link
Contributor

@ppphp ppphp commented Mar 24, 2024

Add override to class derived from ResourceFormatSaver and ResourceFormatLoader. It helps me understand codes.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

This is our general preference and isn't too noisy in my opinion, we might want to add this commit, or a more extensive one, to the blame ignore

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 25, 2024
@akien-mga
Copy link
Member

we might want to add this commit, or a more extensive one, to the blame ignore

I wouldn't, I think the blame ignore should only include cosmetic changes, but this one actually has implications on how code is compiled. If a downstream user has their custom code fail compiling because of this change (missing override in user code), they need to find it in the blame.

@AThousandShips
Copy link
Member

Isn't that compile issue only if you mix it in the same class?

@akien-mga
Copy link
Member

Possibly. Still, it's not a cosmetic change so I wouldn't hide it from blame :)

@akien-mga akien-mga changed the title Add 'override' mark to ResourceFormat class Add override mark to ResourceFormat class Mar 25, 2024
@akien-mga akien-mga closed this pull request by merging all changes into godotengine:master in 0acfb38 Mar 25, 2024
akien-mga added a commit that referenced this pull request Mar 25, 2024
Add `override` mark to ResourceFormat class
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

3 participants