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

Changed Idle's list command to True. In order to stay in convention with MPD #1597

Merged
merged 3 commits into from Jan 28, 2017

Conversation

@Btjones711
Copy link
Contributor

commented Jan 26, 2017

-First bug fix so if there is any corrections or advice please let me know. Criticism is welcome as I am here to learn and help contribute best that I can.

@kingosticks

This comment has been minimized.

Copy link
Member

commented Jan 26, 2017

Hello and thanks for the PR. Have you had a chance to read https://docs.mopidy.com/en/latest/contributing/ ? Particularly the parts regarding setting up a development environment and running the tests? You'll see that the tests have failed for the code changes you've proposed, the test probably just needs updating to reflect your fix.

@adamcik

This comment has been minimized.

Copy link
Member

commented Jan 26, 2017

Since the default is True you can probably just remove argument all together. Other than that look at the travis failure to address the test failure or follow https://docs.mopidy.com/en/latest/devenv/#running-tests

If you want to add some extra polish you can also update docs/changelog.rst with something along the lines of the follow just with ... replaced with text.

v2.1.1 (unreleased)
===================

Bug fix release.

- MPD: ... (Fixes: :issue:`1593`, PR: :issue:`1597`)
@Btjones711

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2017

Hi all,

Thanks for the quick replies! I will look into the tests and thanks for the tip on the default status. First I will look into the contributing document and go through the other steps.

Thanks

@Btjones711

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2017

As for running Mopidy and running the tests I have a windows machine and I see Mopidy is for Linux and Mac environments. Because of this my command line is not running the Mopidy commands, is there any way around this?

Btjones711 added some commits Jan 27, 2017

-Changed Idle command by removing "list_command=False" to stay in con…
…vention with MPD

-Edited change log to reflect the bugfix
@adamcik

This comment has been minimized.

Copy link
Member

commented Jan 28, 2017

Thanks, that looks good to me. Only thing I noticed is my own fault for telling you to add the changelog for 2.1.1 when this is going into develop which will become 2.2.0 or 3.0.0. I'll just followup and pull your two changes over to the release-2.1 branch which eventually becomes 2.1.1 :-)

As for running on Windows we more or less had it working some years ago, but since no one developing Mopidy at the time was using Windows no one has made sure it keeps working. So state now is that it might work, but we don't know how much work it would be to fix it if it doesn't.

Of the top of my head I would say you have to options: 1) Figure out how to run things on Windows or 2) Figure out how to setup a linux VM on your machine. What is easiest (or most interesting to spend time on) is really up to you.

@adamcik adamcik merged commit ebc3eb7 into mopidy:develop Jan 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jodal jodal added this to the v2.2 milestone Mar 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.