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

Travis auto PEP8 cleanup by Persian Prince (not enabled) #2917

Closed
wants to merge 1 commit into from
Closed

Travis auto PEP8 cleanup by Persian Prince (not enabled) #2917

wants to merge 1 commit into from

Conversation

persianpros
Copy link
Contributor

This is not enabled so you could merge and test it yourself, if you check OpenVisionE2/enigma2-openvision@bd67fa6 you'll see the needed changes for .travis.yml

Also you need to set "GH_TOKEN" varialbe for build.sh which you can define it under https://github.com/settings/tokens and add it later to https://travis-ci.org/github/OpenPLi/enigma2/settings so could be used for Travis.

We use this for almost all Open Vision repos, also Open WebIF added it E2OpenPlugins/e2openplugin-OpenWebif@ecd89f2 and ATV is using it for their new images: https://github.com/openatv/enigma2/commits/6.5

My PEP8.sh script proved to be 100% safe after lots of tests me and @IanSav did and we're using it for long, I removed some naughty PEP8 codes so this would be just fine.

Example: OpenVisionE2/enigma2-openvision@74ee9d4

The good thing is if you enable it on Travis you just merge your PRs and it will take care of the problems.

I hope you decide to use it on all of your repos like us.

@Dima73
Copy link
Contributor

Dima73 commented Apr 8, 2021

care of the problems
or create new

@persianpros
Copy link
Contributor Author

@Dima73

It won't, all codes are safe and suspicious ones removed, we're using it for long.

@Dima73
Copy link
Contributor

Dima73 commented Apr 8, 2021

Okay.
If everything is checked.

@persianpros
Copy link
Contributor Author

You can see some more examples here: https://github.com/OpenVisionE2/e2iplayer-ov/commits/master

@WanWizard
Copy link
Member

Personally I'm not a big supporter for anything that automatically changes code.

@betacentauri @athoik @eriksl @littlesat ?

@Dima73
Copy link
Contributor

Dima73 commented Apr 9, 2021

OpenVisionE2/e2iplayer-ov@1601672
it not good

@persianpros
Copy link
Contributor Author

persianpros commented Apr 9, 2021

@Dima73

Why that's not good? It's correct see: https://www.flake8rules.com/rules/E401.html

When we have "from enigma imoport x, y, z" it's ok because we import all from the same thing but why "import re, os" should be on the same line?

@Dima73
Copy link
Contributor

Dima73 commented Apr 9, 2021

Because we have been using it for many years and it works.
Who said that this should ?
https://www.flake8rules.com/rules/E401.html
This link is not law.

@persianpros
Copy link
Contributor Author

@Dima73

Nobody said that won't work, it's just how PEP8 tells and when we can use it why shouldn't we?

Sooner or later all enigma2 repos will start using PEP8 so PLi wants to be the last one or as always be upstream?

The PEP8 changes won't harm.

@betacentauri
Copy link
Contributor

betacentauri commented Apr 9, 2021

In general I'm not the biggest supporter of PEP8. E.g. see the decision for using spaces instead of tabs.
So I also wouldn't say all e2 repos will be PEP8 complaint in future nor it doesn't harm.

Some PEP8 things might be good. Haven't checked the list in this PR yet.
Adding a skript is ok. But I wouldn't execute it automatically and automatically let it commit things.

Why is the script located in the main directory. There is a tools directory for such things.

@persianpros
Copy link
Contributor Author

@betacentauri

My script won't change spaces or tabs.

@betacentauri
Copy link
Contributor

Yes, I know now. And it was only an example, why we shouldn't use all of them.

Having looked at the used PEP8s. I like some (e.g. E701) and don't like some of them (e.g. E251). But it might be my personal taste.

@Dima73
Copy link
Contributor

Dima73 commented Apr 9, 2021

E.g. see the decision for using spaces instead of tabs
Please revert this

@persianpros
Copy link
Contributor Author

@Dima73

Revert what?

@persianpros
Copy link
Contributor Author

@betacentauri

We don't choose because we like or not, we choose what we could apply without breaking things ;)

It's good to follow all PEP8 codes but that may not be possible.

@betacentauri
Copy link
Contributor

@Dima73: PEP8 says „use spaces“. I hope we all here say „that is bad, we want to use tabs”.
I only wanted to say that not all PEP8 requirements are good.

I would like to spend more time into develop new things than in making the code nice. Don’t get me wrong. Code formatting is important. But for me not so important as current code looks ok for me. There are much more important things to do.

@persianpros
Copy link
Contributor Author

As I said my script won't convert tabs to spaces.

This script will keep all repos with a proper standard and all e2 scene could follow it as it won't harm it just makes merges/cherry-picks easier.

@Dima73
Copy link
Contributor

Dima73 commented Apr 10, 2021

As I said my script won't convert tabs to spaces.
Ok again.

@Dima73
Copy link
Contributor

Dima73 commented Apr 10, 2021

I would like to spend more time into develop new things than in making the code nice. Don’t get me wrong. Code formatting is important. But for me not so important as current code looks ok for me. There are much more important things to do.
+1000

@persianpros
Copy link
Contributor Author

It won't take your time if it will be on Travis but just to let you see the changes I sent #2921 which is safe to merge and use.

@Dima73
Copy link
Contributor

Dima73 commented Apr 10, 2021

PEP8 double aggressive E301 ~ E306
3349a10
This not good.It's just not pretty.
Why should we retreat two lines and not one?
Are two lines a law and one a crime?

@persianpros
Copy link
Contributor Author

@Dima73

It's what PEP8 advertise and when we can follow something that won't harm why not? This will help us for any future PEP8 compatibility 👍

Remember we just don't want to accept PEP8 codes that break something, other things are just PEP8 standards and safe to follow.

@WanWizard
Copy link
Member

We've discussed this.

The conclusion is that we don't want something that automatically and without dev intervention alters any code and commits it. If PEP8 standards are a requirement (which hasn't been discussed yet), every author needs to validate the code before issuing a PR, and code violating the standards should not be committed. Autocorrecting sloppy code is or leads to bad practice.

Also, we don't accept any code that has explicit copyright lines in it, all code is copyright by their respective authors by law, there is no need to make it explicit to promote yourself. The same goes for adding "by Persian Prince" to the commit or PR title, it is already visible who the author of a diff is in the git log. As the saying goes, "There is no I in TEAM".

@WanWizard WanWizard closed this Apr 10, 2021
@LraiZer
Copy link
Contributor

LraiZer commented Apr 10, 2021

Every author's future PR could be simply be auto validated for a selected PEP8 conformation before accepting by travis-ci using flake8 without --exit-zero once the initial agreed PEP8 conversion by Persian Prince has been accepeted?

eg. ab flake8

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

5 participants