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

StereoEnhancer code style enhancements #98457

Conversation

betalars
Copy link
Contributor

replacing r l with right and left to increase redability.

I probably wouldn't have done this if it wasn't for a similar commit by @AThousandShips I found while blaming a different file ...

@AThousandShips
Copy link
Member

AThousandShips commented Oct 23, 2024

By my changes do you mean:

That wasn't about style but about matching the bindings, this is a bit different (I in fact left this file without changing these parts when making that change)

@betalars
Copy link
Contributor Author

Oh, I missed the bigger picture. Okay you may discad this then, I think it makes code more readable, but I don't have too strong feelings about this.

@AThousandShips
Copy link
Member

I think it could be worthwhile but I'd say it's still pretty readable from context, but I don't think it needs to be rejected outright

I would say it belongs as part of a broader sweep of readability improvements, like the ones tracked in:

Would be good to open a tracker for things like this as well

@AThousandShips AThousandShips added this to the 4.x milestone Oct 23, 2024
Co-authored-by: K. S. Ernest (iFire) Lee <fire@users.noreply.github.com>
@fire
Copy link
Member

fire commented Oct 24, 2024

Since there isn't many audio maintainers, you might have to do extra work like provide audio clips or anything for us to test that this isn't a regression.

@betalars
Copy link
Contributor Author

@fire wrote

Since there isn't many audio maintainers, you might have to do extra work like provide audio clips or anything for us to test that this isn't a regression.

I mean yes, but you can also check the code manually, because I did not touch the logic intentionally, even tho I saw some reasons to.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

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

That PR is on the line of being not needed per se, but it seems fine. Line by line, nothing seems to have changed.

@akien-mga
Copy link
Member

I would say it belongs as part of a broader sweep of readability improvements, like the ones tracked in:

As suggested here, I merged this changes as part of a bigger commit in #99799, which supersedes this PR.

Thanks for the contribution!

@akien-mga akien-mga closed this Nov 28, 2024
@akien-mga akien-mga removed this from the 4.x milestone Nov 28, 2024
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