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

Adding audiobook script #1407

Closed
wants to merge 7 commits into from
Closed

Adding audiobook script #1407

wants to merge 7 commits into from

Conversation

codeperfectplus
Copy link

@codeperfectplus codeperfectplus commented Oct 15, 2022

Description

Added a script to Read Book in audiobook folder.

Fixes #1406

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines(Clean Code) of this project
  • I have performed a self-review of my own code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have created a helpful and easy-to-understand README.md
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests/screenshots(if any) that prove my fix is effective or that my feature works

@ghost
Copy link

ghost commented Oct 15, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codeperfectplus
Copy link
Author

Please merge it after 31 October.

@ClasherKasten
Copy link
Collaborator

Please merge it after 31 October.

That's possible, but can I ask what the reason is for this request? (Sorry if I am a little too curious).

Copy link
Collaborator

@ClasherKasten ClasherKasten left a comment

Choose a reason for hiding this comment

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

Overall, this is just like an example and documentation for the audiobook library. This is a repository for useful scripts and not a place to promote self-created libraries.
My point here is mainly the README.md. It's just the same as audiobooks README.md. But it shouldn't be that. The README.md for this script should be about the the script that you want to add and not about the library behind it.
You can also read about this in the STYLE_GUIDELINES.md under section "Prerequisites".
If you need inspiration for the README.md take a look at TEMPLATE_README.md file.

script.py itself looks OK to me.

@codeperfectplus
Copy link
Author

Overall, this is just like an example and documentation for the audiobook library. This is a repository for useful scripts and not a place to promote self-created libraries. My point here is mainly the README.md. It's just the same as audiobooks README.md. But it shouldn't be that. The README.md for this script should be about the the script that you want to add and not about the library behind it. You can also read about this in the STYLE_GUIDELINES.md under section "Prerequisites". If you need inspiration for the README.md take a look at TEMPLATE_README.md file.

script.py itself looks OK to me.

Thanks for pointing out the issues. I will make the necessary changes.

@codeperfectplus
Copy link
Author

Please merge it after 31 October.

That's possible, but can I ask what the reason is for this request? (Sorry if I am a little too curious).

I am participating in Hacksquad so I can only make quality PR. Otherwise, I can be disqualified. That's why I was asking to merge it after 31st October.
😁

@ClasherKasten
Copy link
Collaborator

As I reviewed this PR the first time I thought that this would be a very cool addition to the project.
Problem:

I am participating in Hacksquad so I can only make quality PR. Otherwise, I can be disqualified. That's why I was asking to merge it after 31st October.
😁

You state yourself that this is not a quality PR. And even if I think this script would be worth adding, I don't think this project aims to be a collections basin for unqualitative PRs and scripts.
Is there anyone from the other maintainers (@HarshCasper, @iamakkkhil, @sohamsshah, @seema1711, @vybhav72954, @RohiniRG) who has an (other) opinion on this?

@codeperfectplus
Copy link
Author

codeperfectplus commented Oct 15, 2022

but it's a very helpful script to convert the book to audio format. so, In my opinion, it's worth it. I mentioned the quality PR which means contributions to the collections repo and DSA repo are not allowed

@ClasherKasten
Copy link
Collaborator

ClasherKasten commented Oct 15, 2022

I mentioned the quality PR which means contributions to the collections repo and DSA repo are not allowed

Following this argument, you already should be disqualified for opening PRs in awesomeScripts as this is basically the same as this project. So I personally don't see any reason for not merging it before 31. October.

@codeperfectplus
Copy link
Author

I mentioned the quality PR which means contributions to the collections repo and DSA repo are not allowed

Following this argument, you already should be disqualified for opening PRs in awesomeScripts as this is basically the same as this project. So I personally don't see any reason for not merging it before 31. October.

I got warnings for that. No issue you can merge it.

@ClasherKasten
Copy link
Collaborator

I got warnings for that. No issue you can merge it.

I didn't no this. Now the situation is understandable, I guess this doesn't gonna get merged before 31st October.

@codeperfectplus
Copy link
Author

I got warnings for that. No issue you can merge it.

I didn't no this. Now the situation is understandable, I guess this doesn't gonna get merged before 31st October.

Thanks @ClasherKasten

@codeperfectplus codeperfectplus closed this by deleting the head repository Jan 20, 2023
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.

[feature request]:
2 participants