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

Prettier readme #1006

Merged
31 commits merged into from Dec 22, 2020
Merged

Prettier readme #1006

31 commits merged into from Dec 22, 2020

Conversation

phcreery
Copy link
Contributor

No description provided.

@phcreery
Copy link
Contributor Author

phcreery commented Dec 1, 2020

Is there any information on audio quality I should add?

@ghost
Copy link

ghost commented Dec 4, 2020

Just say that we pull audio quality from YouTube. So the max you get is 128kbps.

Also mention that YTM should be available in your country to use spotDL

@ghost
Copy link

ghost commented Dec 4, 2020

Also adds few tags from shield.io

@phcreery
Copy link
Contributor Author

phcreery commented Dec 4, 2020

Okay. Yea I already have tags and the YTM note

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

# Installation
You need to download ffmpeg to use this tool. Download and installation instructions can be found at [ffmpeg.org](https://ffmpeg.org/)
Copy link
Member

Choose a reason for hiding this comment

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

For simplicity for the end user, some of whom may not be as technologically savvy, the original readme's links should be used

  1. MacOs
  2. Windows
  3. Linux
  4. Central Release Page

Directing users to the ffmpeg.org website adds space for error

See highlighted CORRECT link for windows, but most users would probably go for the big green "Download" button at the top of the page.
image

Copy link
Member

Choose a reason for hiding this comment

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

Also change ffmpeg to FFmpeg as per FFmpeg's style guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if that helps a whole lot. If the user is unable to navigate the FFmpeg site, then I doubt they would be able to navigate the Windows site. Plus, since those are unofficial builds, I imagine they are subject to change.

Copy link
Member

Choose a reason for hiding this comment

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

Hm understood. Check with @Mikhail-Zex for the final call ig :)

Copy link
Member

Choose a reason for hiding this comment

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

But do change ffmpeg to FFmpeg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Silverarmor Thanks for noticing, will do.
@MikhailZex what do you think?

Copy link

Choose a reason for hiding this comment

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

I don't think both are exceedingly important. Do what you think is correct.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Dec 5, 2020

Nice to see jumping around already @Silverarmor

@Silverarmor
Copy link
Member

Nice to see jumping around already @Silverarmor

Translation / Proofreading / documentation / admin in PRs and issues is my specialty haha, actual coding not so much, but working on improving my coding :)

@ghost
Copy link

ghost commented Dec 5, 2020

Could ya write up docs for spotdl then @Silverarmor ?

@Silverarmor
Copy link
Member

Could ya write up docs for spotdl then @Silverarmor ?

I'd be happy to, Exams finish tomorrow for me, then I'm off school.
What format would you want them in? like full on docs/wiki/etc.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

# Installation
You need to download ffmpeg to use this tool. Download and installation instructions can be found at [ffmpeg.org](https://ffmpeg.org/)
Copy link
Member

Choose a reason for hiding this comment

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

But do change ffmpeg to FFmpeg

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@ghost
Copy link

ghost commented Dec 7, 2020

Could ya write up docs for spotdl then @Silverarmor ?

I'd be happy to, Exams finish tomorrow for me, then I'm off school.
What format would you want them in? like full on docs/wiki/etc.

I'll hit you up by evening.

@phcreery
Copy link
Contributor Author

phcreery commented Dec 13, 2020

@Silverarmor I agree, I was updating mine to reflect the changes @MikhailZex did at b9d55ff
I will put it back in

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Dec 16, 2020

Plese keep .venv as an option for developers. After all its for them to decide what to do. We can retain the .venv instructions for those who don't know about it but also add straightforward non-venv instructions

@phcreery
Copy link
Contributor Author

Plese keep .venv as an option for developers. After all its for them to decide what to do. We can retain the .venv instructions for those who don't know about it but also add straightforward non-venv instructions

Okay, that is better.

@Silverarmor
Copy link
Member

@aklajnert to re-review ASAP, then PR is ready to merge

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Few minor notes. All good otherwise.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

👍

@phcreery
Copy link
Contributor Author

Good deal. Thanks for y'alls help. Since @MikhailZex has the write permissions, just waiting on your call.

@ghost
Copy link

ghost commented Dec 21, 2020

Will merge asap.

@ghost ghost merged commit eb8095f into spotDL:master Dec 22, 2020
@ghost
Copy link

ghost commented Dec 22, 2020

Well, great job guys. I just realized this PR has over 68 comments - shows the thought, time and dedication put in. Thank you.

This pull request was closed.
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.

None yet

3 participants