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 beatloop_activate to beatloop_toggle #4519

Closed
wants to merge 1 commit into from

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Nov 13, 2021

beatloop_toggle makes more sense with the new behavior of
deactivating the current loop if pressed when looping is enabled.

Follow up of #4328

beatloop_toggle makes more sense with the new behavior of
deactivating the current loop if pressed when looping is enabled.
@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 13, 2021

I think we should ignore the pre-commit errors for this mass renaming PR.

@Swiftb0y
Copy link
Member

Do we really need this mass renaming? We have to support the beatloop_activate CO anyways for a long time since there are skins and mappings outside our control. IMO updating the manual is sufficient. We can still merge since you already did most of the work, but for the future, I'd like to avoid these mass renaming PRs.

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 13, 2021

Strictly speaking no, we do not need the mass renaming because of the aliasing. But I think it is better to have everything included in the repository consistent. Aliasing is needed just for backwards compatibility with unofficial controller mappings and skins that were never merged.

@ronso0
Copy link
Member

ronso0 commented Nov 14, 2021

Aliasing is needed just for backwards compatibility with unofficial

...customized controller mappings and skins.

@ronso0
Copy link
Member

ronso0 commented Nov 14, 2021

I think it is better to have everything included in the repository consistent

I agree. Document it in the manual, then let's merge.

@Holzhaus
Copy link
Member

I disagree. There is no reason to rename, just document the new behavior. activate does not mean enable, it is similar to hotcue_X_activate IMHO.

@Swiftb0y
Copy link
Member

There is no reason to rename

Not really, but the work has already been done so denying a merge is pointless and discouraging towards Be's efforts.

@ronso0
Copy link
Member

ronso0 commented Nov 14, 2021

There is no reason to rename

Not really, but the work has already been done so denying a merge is pointless and discouraging towards Be's efforts.

well, that argumentation could be abused for controversial PRs.
Actually for mass-renaming a prior discussion should be required.

@Swiftb0y
Copy link
Member

well, that argumentation could be abused for controversial PRs.

I agree, but I don't consider this change controversial. Though I can understand if you don't want to accept that argument.

@ronso0
Copy link
Member

ronso0 commented Nov 14, 2021

In this particular case I don't mind.
It's just that we should agree on mass renamings before doing the work, running into merge conflicts continously, time pressure to merge or not, having heated discussions etc. which only unnecessarily draws energy. I think we had all that too often already.

@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 14, 2021

I wouldn't really be upset if this is rejected. It didn't take much work to implement either. 🤷

@ronso0
Copy link
Member

ronso0 commented Nov 14, 2021

That's why "In this particular case I don't mind." ;)

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.

4 participants