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

feat: add support for device-type #500

Merged
merged 4 commits into from
Feb 9, 2020
Merged

Conversation

wdehoog
Copy link
Contributor

@wdehoog wdehoog commented Feb 6, 2020

See Issue #450.

Sorry for all the git noise. I am unfit for git. For rust as well.

@mainrs
Copy link
Member

mainrs commented Feb 6, 2020

The whole git noise is no problem. I usually squash all commits into one (merging them) before merging into master. Better to split them off into smaller ones that are easier to follow then having one huge commit. I would still consider this commit small so don't worry :)

Could you run cargo fmt as well as cargo clippy? The first formats the code and the second command runs some linting. Usually you get some warnings displayed in your terminal with suggestions on how to fix them. Quite straight forward.

Would be nice if you could do that so the CI passes 😄

I wouldn't consider myself good writing Rust code either. There are a lot of people who are way better at it than me. But you have to start somewhere and open source is a great place to gather experience for languages you are not that familiar with :)

@mainrs
Copy link
Member

mainrs commented Feb 6, 2020

They should both be installed if you use the stable branch. If not, rustup component add clippy rustfmt should do the trick.

I will merge it on the weekend as I don't have much time right now :)

@wdehoog
Copy link
Contributor Author

wdehoog commented Feb 6, 2020

Thanks. I'll run these tools. I tried to keep the formatting the same as already present. This fmt tool seems to disagree. Do you prefer the way fmt advises?

@mainrs
Copy link
Member

mainrs commented Feb 6, 2020

Yep, we always run cargo fmt. Took a glimpse it seems that the only problem was that your imports were not alphabetically sorted, which should cargo fmt do for you :)

In the feature this will be done automatically (#501).

@mainrs mainrs added this to the v0.2.25 milestone Feb 9, 2020
@mainrs mainrs changed the title add support for device-type feat: add support for device-type Feb 9, 2020
@mainrs mainrs merged commit b690531 into Spotifyd:master Feb 9, 2020
@mainrs
Copy link
Member

mainrs commented Feb 9, 2020

Thanks!

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.

2 participants