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 warning when trying to enable AutoDJ with left & right deck #11018

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

MetriSomesh
Copy link
Contributor

Added warning message when trying to enable AutoDJ if there's no left & right deck.

@ronso0
Copy link
Member

ronso0 commented Oct 28, 2022

Please stop opening new PRs, instead follow the instructions @daschuer and I gave you and fix the original PR. This is actually trivial given the small change set.
First get familiar with how git works. Not doing that and not reading the hints git gives in the terminal and thereby implicitly` asking for help for every single step is a waste of developer time.

To be continued in #11013

@MetriSomesh
Copy link
Contributor Author

@ronso0 Sorry about that I will not repeat my mistakes again, I understood the problem and the pre-commit was causing it I don't know how. But at the time of commit normally it ask for password, but while using pre-commit it doesn't ask any password, that's why it is causing errors

@MetriSomesh
Copy link
Contributor Author

@ronso0 You can check this request for the checks If any error will occur I will restudy the whole github and then I will start contributing to this community

@ronso0
Copy link
Member

ronso0 commented Oct 28, 2022

Pre-commit is still failing since the indentations don't match.
You need to install pre-commit as explained in using Git https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking
Then pre-commit will be run on the changed files when you git commit (and that will not ask for your password, only git push will)

@MetriSomesh
Copy link
Contributor Author

MetriSomesh commented Oct 28, 2022

@ronso0 I have installed it already. Ok I will fix this completely first then I will try to commit or to push a request thank you very much

@MetriSomesh MetriSomesh marked this pull request as draft October 28, 2022 12:02
@MetriSomesh
Copy link
Contributor Author

MetriSomesh commented Oct 28, 2022

@ronso0 I am sorry to disturb you again but I was looking for this from nearly 4 hours now still cant find a way to fix it

The pre-commit is giving this output:

err

It doesn't fixing the format just getting closed immediately after this I checked git status as well no changes are show

@ronso0
Copy link
Member

ronso0 commented Oct 28, 2022

You obviously didn't bother to read the command output. Below the red Failed it says
"Python was not found"...

@daschuer
Copy link
Member

There is work in progress to make pre-commit on windows more robust. Please

git pull git@github.com:daschuer/mixxx.git pre-commit-python

Unfortunately this will combine the two PRs, but I can clean that up before merge.

@daschuer
Copy link
Member

You may also report your experience in the original PR that we get it merged soon:
#11004

@ronso0
Copy link
Member

ronso0 commented Oct 28, 2022

You obviously didn't bother to read the command output. Below the red Failed it says "Python was not found"...

Ok, sorry, didn't know this is a known issue on Windows.
Please proceed as @Holzhaus suggested on Zulip https://mixxx.zulipchat.com/#narrow/stream/247620-development-help/topic/Pre-commit-help/near/306704977
Download the pre-commit patch file, apply it locally, git commit --amend and force-push.
Then this should finally be in a mergeable state.

@MetriSomesh
Copy link
Contributor Author

@ronso0 I have followed all the steps as you followed

@MetriSomesh
Copy link
Contributor Author

MetriSomesh commented Nov 1, 2022

@daschuer I am sorry, I didn't opened my github because I was searching solution on the error which was given by the pre-commit. I have now seen the PR you mentioned. Now I will install and share my experience on #11004

@ronso0 ronso0 changed the title Add warnign when trying to enable AutoDJ with left & right deck Add warning when trying to enable AutoDJ with left & right deck Nov 1, 2022
@ronso0
Copy link
Member

ronso0 commented Nov 1, 2022

Okay, finally LGTM
@daschuer any more comments?

This is a good first step. Later on it would be nice to show a message when assign switches are changed while AutoDJ is running. Though I think this is out o scope here.

@MetriSomesh
Copy link
Contributor Author

@ronso0 @daschuer Thank you very much for your help. I learned a lot of things while doing my first ever git request successful.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you for your endurance!

Before merge, we need your permission to distribute the code with Mixxx.
Please sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ and comment here when done. Thank you.

@daschuer daschuer added changelog This PR should be included in the changelog and removed build code quality labels Nov 2, 2022
@MetriSomesh
Copy link
Contributor Author

MetriSomesh commented Nov 3, 2022

@daschuer Done

@daschuer daschuer merged commit e561dce into mixxxdj:2.3 Nov 3, 2022
@MetriSomesh
Copy link
Contributor Author

@ronso0 and @daschuer Thank you very much.

What is LGTM anyway ?

@daschuer
Copy link
Member

daschuer commented Nov 3, 2022

LGTM= Looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog library ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants