YT-Spammer-Purge
YT-Spammer-Purge copied to clipboard
ThioJoe
Reopen: Add Black formatting
Related Issue/Addition to code
- Fixes #
- Code readability issues
- Ensures the repository follows the PEP8 standard
- Remove unnecessary line from CONTRIBUTING.md -- "Please don't make a pull request with a bunch of changes in syntax just for the sake of 'best practices' [...] Unless something makes a difference to performance frankly I don't care." -- There are plenty of PRs that get accepted that do not make the slightest difference to performance, and this line pushes away many from contributing. ( #194 : "So I guess I won't bother. Too bad." #489 : "I personally don't think this project at this state is salvageable, especially given ThioJoe's attitude." )
Type of change
- [x] New feature (non-breaking change which adds functionality)
- [x] This change requires a documentation update
Proposed Changes
- Add workflows to automatically format and stylize the code using Black
-
- This helps to ensure all code is properly formatted with Black, regardless of where it is edited.
- Add .pre-commit-config.yaml for developers with the pre-commit tool to format their code before they create a pull request.
- Update CONTRIBUTING.md with a note about Black ( Some changes from #489 : @dekoza , and some from #464 's comments : @rachmadaniHaryono )
- Adds instructions to setup a development environment
Checklist:
- [x] My code follows the style guidelines of this project and have read CONTRIBUTING.md There are no clear style guidelines. This adds some.
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have checked my code and corrected any misspellings
Additional Info
See discussion #194
Black does NOT change any of the underlying code itself, but can help make obvious errors and bugs more visible. Black does NOT fix design problems in the project, but can help highlight areas that need improvement. Black does NOT need to be installed to run the code -- It is only used during development. If the code is easier to read, it is easier to change. Is Black safe to use?
Pylint Score before Black formatting: -9.18/10 Pylint Score after Black formatting: 0.98/10 The repository would still need big improvements, but a difference of 8.2 is huge.
Why does this matter? Brief tutorial on Pylint
This is a reopen of #464 -- I believe the changes in this are valuable enough to be carefully considered, not denied due to a lack of understanding. Please carefully consider each PR. As you make clear, you may not be an expert, but it does not mean you cannot improve. Contributors are trying to improve the project. Whether or not Black is the chosen formatter for this project doesn't matter much to me, as long as one is decided upon, and it doesn't cause too much trouble to use. Similar to testing, readability is important to enforce and will help improve the quality of the code.
Please ignore the failing tests, the codebase in its current state obviously does not adhere to the Black style guidelines, which is where this fail is from.
some response here https://github.com/ThioJoe/YT-Spammer-Purge/discussions/194#discussioncomment-2118124
should i copy my message here?
1c5f35d61eb9fd98b4d8e4fbd88c6b67f7f62b6a The CONTRIBUTING.MD is fine as it is.
I mean, it's fine as it is, according to me.
On Mon, 7 Feb 2022 at 00:45, Ethan Hindmarsh @.***> wrote:
@.**** commented on this pull request.
In CONTRIBUTING.md https://github.com/ThioJoe/YT-Spammer-Purge/pull/573#discussion_r800221659 :
-- Avoid adding new non-standard libraries if at all possible. If you have an idea that would require one, please suggest it as an issue first instead of going through all the work and submitting a pull request. +- Avoid adding new non-standard libraries if at all possible. If you have an idea that would require one, please suggest it as an issue first instead of going through all the work and submitting a pull request.
Why not? Again, emphasis.
— Reply to this email directly, view it on GitHub https://github.com/ThioJoe/YT-Spammer-Purge/pull/573#discussion_r800221659, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUGJUTO2IXJUYEI7XGU4RMDUZ3CFXANCNFSM5NUPN6RA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you commented.Message ID: @.***>
I mean, it's fine as it is, according to me.
Sure it's fine, but it could be better.
I mean, it's fine as it is, according to me.
Sure it's fine, but it could be better.
true
Tbh I don't really have any intention of adding this, I don't really understand the benefit
Comment by ThioJoe
I believe adding formatting of any sort deserves a second thought, there are plenty of benefits. I don't see it as a reason to close the PR as I said in the PR description,
This is a reopen of https://github.com/ThioJoe/YT-Spammer-Purge/pull/464 -- I believe the changes in this are valuable enough to be carefully considered, not denied due to a lack of understanding.
Please carefully consider each PR. As you make clear, you may not be an expert, but it does not mean you cannot improve. Contributors are trying to improve the project.
Whether or not Black is the chosen formatter for this project doesn't matter much to me, as long as one is decided upon, and it doesn't cause too much trouble to use.
Similar to testing, readability is important to enforce and will help improve the quality of the code.
Dude chill out, its not your repo. If you don't like a choice you don't have to deal with it lol
Comment by Masamune3210
This comment was directed to someone else, not towards the PR or myself
Duplicate of https://github.com/ThioJoe/YT-Spammer-Purge/pull/464 , ThioJoe, doesnt want to add it, its his project and his wish, so please don't create multiple PR's on the same thing
Read the PR description. Thio did not say he is against code formatting, only that he did not understand the benefit.
[Merge ](/ThioJoe/YT-Spammer-Purge/pull/573/commits/7de772db3228abf166693423a823692d6f9ea443)
👍
well, i agree but its @thiojoe's project, he can make whatever guidelines he wants, you've shared your opinion though.
On Mon, 7 Feb 2022 at 01:00, Ethan Hindmarsh @.***> wrote:
@.**** commented on this pull request.
In CONTRIBUTING.md https://github.com/ThioJoe/YT-Spammer-Purge/pull/573#discussion_r800223124 :
-- Please don't make a pull request with a bunch of changes in syntax just for the sake of 'best practices' (ex: changing "if blah == False" to "if not blah"). Unless something makes a difference to performance frankly I don't care. +- Please ensure all code has been formatted by Black (Usage Guide). This helps to maintain code readability and keep a consistent style across all files. This is optional for you to do.
If a PR is a tiny change with no difference, it will be denied. Ex. Change is to == This should be common sense; The line should therefore be redundant, aside from making the project seem hostile to small change. If one cannot make a big change, and one cannot make a small change, what is one to do?
— Reply to this email directly, view it on GitHub https://github.com/ThioJoe/YT-Spammer-Purge/pull/573#discussion_r800223124, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUGJUTMBDXQDA5NGE2QTMFLUZ3D5LANCNFSM5NUPN6RA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you commented.Message ID: @.***>
I'll consider it, but I'm not going to make a decision either way unless I fully understand it. So no guarantees
just today django just merged pr to autoformat their codebase with black https://github.com/django/django/pull/15387
from that pr there is related deps that explain why they do that https://github.com/django/deps/blob/main/accepted/0008-black.rst
from that pr there is related deps that explain why they do that https://github.com/django/deps/blob/main/accepted/0008-black.rst
The explanation provided there is very compelling. It saves everyone the headache of thinking about formatting, makes bugs more readily apparent, and makes it easier for new contributes who don't know how the rest of the project was formatted.
https://github.com/ThioJoe/YT-Spammer-Purge/discussions/194#discussioncomment-2118124
it does look fine but maybe we can use github action rather than automated commit from an user
from failed github action
remote: Permission to [ThioJoe/YT-Spammer-Purge](https://github.com/ThioJoe/YT-Spammer-Purge/).git denied to github-actions[bot].
fatal: unable to access 'ThioJoe/YT-Spammer-Purge': The requested URL returned error: 403
Error: Process completed with exit code 128.
something is changed from github action
RE: rachmadani I don't know too much about GitHub actions (or workflows), but if it does the same job then LGTM✅
As per the failed GitHub action, it is due to
- it attempting to automatically push to main when it has not yet received permission to edit files
- the repo not being black formatted (causing #1)
I'd like to reiterate, Any formatting standard would at least set a consistent format for the code, implementation details don't matter to me😁 The sooner one can be chosen and implemented the better👍
As per the failed GitHub action, it is due to
- it attempting to automatically push to main when it has not yet received permission to edit files
- the repo not being black formatted (causing # 1)
it is number 1
https://github.com/ThioJoe/YT-Spammer-Purge/runs/5225509511?check_suite_focus=true
e:
from failed github action
...
something is changed from github action
i was wrong, i thought there is change on github action behavior between your first post and this time but i think it is because the commit for new github action activate after first post
success check 87402e0 (#573)
first failed check 75003b9 (#573)
I'll consider it, but I'm not going to make a decision either way unless I fully understand it. So no guarantees
OK, so basically python gives out standards on how to make your code look readable and under the Python standard. One of this is PEP 8. Black is a formmatter witch basically takes your code, format's it for PEP 8 and that's it. Adds no overhead and just and extra command when you need to interpret.
Edit
Not compile, interpret
Also, completely support changing that line in contributing... stooped me from opening a pr once!
Does this have chances to be added? Even without the removing in contributing.md?
Does this have chances to be added? Even without the removing in contributing.md?
🤷 Unless there are any changes requested or suggested, the PR is ready for review whenever. The changes are small enough that there shouldn't be any merge conflicts even after a month
The PR is honestly just waiting for @ThioJoe to make time for a review👍
I've been using this code formatted with Black (manually) for about a month now with no problems. I haven't tried to automate it or test using actions but in principle, I completely support this.
RE: Kendall
Sure, those would probably be better suited to it, but I can only submit changes to CONTRIBUTING.md 😅 A basic environment setup for new contributors wouldn't hurt though, wherever it may be
You can't move the stuff to README then? or put in the PR description, It would be good if this section gets added to wiki? I mean no harm in CONTRIBUTING.md but it would be better in wiki or readme.
Bro this has been open for so long now , fully support this, and as i said @ThioJoe look at the repo for it black