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

Replace setting by reference with ties #100445

Closed
wants to merge 1 commit into from

Conversation

tygyh
Copy link
Contributor

@tygyh tygyh commented Dec 15, 2024

Shortens the method signature and make the code smaller. Pure positives!

@tygyh tygyh requested a review from a team as a code owner December 15, 2024 18:30
@tygyh tygyh force-pushed the Refactor-initialization branch from 41bd7ea to 711da70 Compare December 15, 2024 19:20
@tygyh tygyh force-pushed the Refactor-initialization branch from 711da70 to bddbf5e Compare December 15, 2024 19:46
@tygyh tygyh changed the title Replace setting by reference with structured bindings Replace setting by reference with ties Dec 15, 2024
@RedMser
Copy link
Contributor

RedMser commented Dec 15, 2024

@arkology You probably wanted to refer to the "Standard Template Library" headline, not the one regarding the "auto" keyword? ^^

But yeah, Godot codebase does not accept std::tuple, so this change will likely not be accepted.

@arkology
Copy link
Contributor

arkology commented Dec 16, 2024

@RedMser previous the author also used auto keyword)
I believe that if the author wanted to contribute something useful with their PR, they would have read the entire page after being pointed out one of its headlines. It's okay not to know all the rules (I don't 🙃), but a link to them was provided.

@tygyh
Copy link
Contributor Author

tygyh commented Dec 16, 2024

if the author wanted to contribute something useful with their PR, they would have read the entire page

I don't see any mentions of tuples in the rules. I can update the docs to include an explanation of why they aren't allowed if you explain the reason to me.

@arkology
Copy link
Contributor

arkology commented Dec 16, 2024

I'm really sorry if it sounds offensive. You are right, the docs don't mention it directly. Feel free to open an issue on the Godot Docs repo.

@tygyh
Copy link
Contributor Author

tygyh commented Dec 16, 2024

I'm really sorry if it sounds offensive.

No offense taken. You were explaining the spirit of the rules and it simply didn't match the letter of the rules. Easy fix!

@tygyh
Copy link
Contributor Author

tygyh commented Dec 16, 2024

I've made an issue mentioning this. Since this change isn't allowed I'll close this PR.

@tygyh tygyh closed this Dec 16, 2024
@akien-mga akien-mga removed this from the 4.x milestone Jan 8, 2025
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.

5 participants