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

Rename "FX-Mixer" to "Mixer". #4890

Closed
wants to merge 7 commits into from
Closed

Conversation

claell
Copy link
Contributor

@claell claell commented Mar 12, 2019

Closes #3300 (hopefully, if not sorry for wasting your time!).

I currently only renamed String texts imho, but can also rename classes, variable names etc.

Not tested.

@josh-audio
Copy link
Member

Ordinarily we'd target changes like this to master, but I see no reason not to target 1.2.

Also while you're at it, I'd personally be for changing variable names, though this is significantly more error-prone. Also I'm not as regular of a contributor as some others, so I'll let someone else weigh in on that.

@claell
Copy link
Contributor Author

claell commented Mar 12, 2019

Ah, alright, makes sense. Can I change this now to target to master?

Will also change variable names etc.

@josh-audio
Copy link
Member

Before you target master, let's make sure nobody has objections to targeting 1.2. Changes to 1.2 are regularly merged into master.

You can't just change the target to master without breaking a lot. You would have to:

  1. Clone master
  2. Re-apply your changes
  3. Create a new PR

@claell
Copy link
Contributor Author

claell commented Mar 12, 2019

Ok. Have created some more commits now to hopefully rename all instances of FX Mixer. I split it up into many commits to make it easier to follow the search and replace changes.

@tresf
Copy link
Member

tresf commented Mar 12, 2019

stable-1.2 is bug-fix only. Please re-target.

Not tested.

All changes should be tested. If not, you're asking the developers to test for you, which is asking them to work on a task you invented.

I currently only renamed String texts imho, but can also rename classes, variable names etc.

Why cause inconsistencies in the codebase?

@claell
Copy link
Contributor Author

claell commented Mar 12, 2019

stable-1.2 is bug-fix only. Please re-target.

Alright.

All changes should be tested. If not, you're asking the developers to test for you, which is asking them to work on a task you invented.

Well, I did not invent it, but ok. Is there a build artifact that I can use for easy testing?

Why cause inconsistencies in the codebase?

I don't know, I just heard that FX Mixer should be renamed to Mixer, so I thought why not go all the way. If this is not wanted, just say. Then I will try to only rename the relevant Strings.

@tresf
Copy link
Member

tresf commented Mar 12, 2019

If this is not wanted, just say

Why cause inconsistencies Please do not cause inconsistencies in the codebase.

@claell
Copy link
Contributor Author

claell commented Mar 12, 2019

Closing to create another PR for master.

@claell claell closed this Mar 12, 2019
@claell claell deleted the fx-mixer-to-mixer branch March 12, 2019 17:05
@tresf
Copy link
Member

tresf commented Mar 12, 2019

FYI, PRs can also be retargeted. It can get messy with largely-diverged branches like ours, so this is just for information purposes. :)

image

@claell claell mentioned this pull request Mar 13, 2019
@tresf tresf mentioned this pull request Mar 13, 2019
@claell claell mentioned this pull request Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants