Code Quality

[diegogangl]

This issue is related to small things that can be done to improve code quality and make it more approachable to new contributors. This is an on-going effort.

Organize

• [X] Put all .ui files in one place
• [X] Rename and organize .py files in gtk (following a consistent system)
• [X] Move tools into core, put in appropiate sub-packages
• [X] Move info.py to core and update

Modernize

• [X] Rename manager module and class to application
• [X] Make application extend GtkApplication
• [X] Remove PID detection code
• [X] Port actions/callbacks to GActions
• [X] Port context menus to GMenu
• [x] Use a dev Flatpak for development
• [x] Use Meson to build
• [ ] Use GTK resources to manage and load ui and css files
• [ ] Use Gtk.Template decorator to load ui files (when possible?)
• [X] Replace old % formatting with f-strings
• [X] Remove Python2-isms, like class Something(object)

Clean-up

• [X] Simplify logger class into a module
• [ ] Look around for functions that aren't used anymore (due to GUI changes)
• [ ] Add Pylint as a commit hook to keep a consistent style and check for code smells (we need to make a pylintrc file first, since it's way too strict)
• [ ] Look around for TODOs/FIXMEs/etc and either remove them, fix them or add them as a ticket here
• [ ] Make a branch and clean up pylint errors one by one
• [x] Remove old icons not being used anymore

[nekohayo]

That all makes sense! Thanks for laying out the plan for it all.

If I may add my 2¢ of peanut gallery opinion, I would like to suggest officially dumping the prehistoric "Manually wrap anything that exceeds 72 or 79 characters/columns" rule, but otherwise respecting PEP8 in general.

When I peeked at the GTG codebase a few months ago, the code was filled with those over-zealous linebreaks. I personally think the 72/79-columns rule is stupid (hey, we're not running 640x480 screens anymore, folks!) as it introduces extra work and hurts code readability more than it helps in this millenium.

In Pitivi we had agreed upon my proposed "compromise" approach (wrap if the line is really unbearably long, otherwise try to keep it on one line), as documented here: https://gitlab.gnome.org/GNOME/pitivi/blob/master/docs/Coding_style_guide.md

• If you also think manually breaking lines at arbitrary positions (instead of truly semantic positions or not at all) is ugly, we could make it a code style recommendation in GTG as well (I guess it would just be a matter of documenting that policy and slacking off the pylint checker config and such). Since I'm not the one who has to wrestle with the majority of the codebase I'm merely suggesting here of course, but I do think it makes sense.
• Hint: if you want to save some time, you could totally steal the pre-commit hooks and pylint configs linked from the intro of that Pitivi code style documentation page ;)

[diegogangl]

When I peeked at the GTG codebase a few months ago, the code was filled with those over-zealous linebreaks. I personally think the 72/79-columns rule is stupid (hey, we're not running 640x480 screens anymore, folks!) as it introduces extra work and hurts code readability more than it helps in this millenium.

The 79 columns limit is really useful when you want to have two files open side by side in 1080p screen. I do agree that we shouldn't have non-semantic line breaks, a little scrolling isn't going to kill anyone :)

Hint: if you want to save some time, you could totally steal the pre-commit hooks and pylint configs linked from the intro of that Pitivi code style documentation page ;)

Aren't those made to run inside the Pitivi sandbox/flatpak env?

[nekohayo]

Aren't those made to run inside the Pitivi sandbox/flatpak env?

Oh, that may be possible, I haven't looked at this stuff in years ;) I thought those could just be used almost as-is.

[aleb]

Aren't those made to run inside the Pitivi sandbox/flatpak env?

pylint must be run in the sandbox, because it needs access to the modules imported by the checked files. But it could be run out of the sandbox just as well, if you take care of all the deps.

The other checks you can see in .pre-commit-config.yaml are actually run in virtual envs set up by the pre-commit.com tool we use. (Technically they do run in the sandbox, but since they run in custom venvs, they can run anywhere.)

The page is http://developer.pitivi.org/Coding_style_guide.html — make sure to check all the links at the top.

[aleb]

See https://gitlab.gnome.org/GNOME/pitivi/merge_requests/216/commits for how we cleaned up the codebase one error/warning at a time.

Recently I was reading about flake8 which checks the "style" vs pylint which checks the "correctness" if I understand correctly, and I added flake8 to the list of checks, and it turns out after the huge pylint cleanup it passes, no need to fix anything else or add #noqa in the codebase to ignore flake8 warnings. Currently it's run before pylint because it runs much faster, this means it has a chance to complain first—hopefully it won't be too bad.

[aleb]

We're using pre-commit.com to run the checks, BTW.

[diegogangl]

Hey @aleb! Thanks for the tips. Doing the cleanup on a branch one-by-one seems like the best way. Yeah, I understand flake8 only checks for pep8 correctness. Really like the idea of pre-commit.com too, though I think we could just start with a bash script running pylint.

[liberforce]

Flake8 doesn't only check PEP8 conformance. Flake8 is a frontend for pycodestyle (which checks PEP8), pyflakes (which checks validity), and mccabe (checks algorithmic complexity).

Pylint is a linter, not a validator like pyflakes, so it will comment on style and add extra checks, at the cost of speed, like @aleb said.

See Difference between a validator and a linter and the journey of a developer migrating from pylint to flake8.

[diegogangl]

Hey @liberforce thanks for the info. I had the impression Flake8 was just a better pep8, but there's a lot more in there. It even has plugins! Maybe we could replace pylint with Flake8 + Bugbear (plugin)

[liberforce]

Never tried flake8 plugins (only started using pyflakes, then flake8 a few weeks ago). But the whole point is to have flake8 integrated with your editor, so you can have checks as you type, which is not possible with pylint for speed reasons, so check the flake8 plugins don't slow down your setup too much.

Editor integration uses plugins specific to your editor. For vim for example, I've heard vim-ale is the best choice, but I've not tried it yet, though.

[liberforce]

If I may add my 2¢ of peanut gallery opinion, I would like to suggest officially dumping the prehistoric "Manually wrap anything that exceeds 72 or 79 characters/columns" rule, but otherwise respecting PEP8 in general.

When I peeked at the GTG codebase a few months ago, the code was filled with those over-zealous linebreaks. I personally think the 72/79-columns rule is stupid (hey, we're not running 640x480 screens anymore, folks!) as it introduces extra work and hurts code readability more than it helps in this millenium.

My humble opinion on this take is: switch to either yapf or black, and use either CI checks on code formatting or pre-commit checks to have a uniform coding style across the whole codebase. No more questions, just chose a convention and stick to it. Then about the number of columns, this is not a 640×480 thing, having a large screen and a small code width allows to have more files side-by-side when you're coding. The longer the lines, the less files you can fit, and that can get annoying. Plus, having short lines helps when doing some diffs, as with one parameter per line, you have very readable and small diffs. yapf can use some hints to force that behavior, and black... well, is black and does only one thing. Having too many indentation levels is also a sign of some bad patterns, like too many nested ifs, and short lines will bite that coding style sooner, which is a good thing. For long strings, use string concatenation inside parenthesis.

[diegogangl]

But the whole point is to have flake8 integrated with your editor, so you can have checks as you type

Yeah, I already do that but the point in having it for a hook/CI is to enforce consistency for every contribution.

My humble opinion on this take is: switch to either yapf or black, and use either CI checks on code formatting or pre-commit checks to have a uniform coding style across the whole codebase.

We'll definitely support PEP8. But maybe with one or two deviations (line length up to 90-ish and 2 line breaks for methods)

I'm not in favor of autoformatting in a hook or CI though. We could try it once the codebase is more consistent, but I think it could lead to a lot of unwanted extra changes in PRs. It'd be better to abort the commit and ask the author to fix the errors.

[liberforce]

Yeah, at work we shifted to black with 90 columns per line a few months ago, which I found to be a good compromise. If you totally want to avoid this style issue, black is nice. There is no deviation, and sometimes the formatting may look a bit weird as you have no control on any other setting than line length (while yapf is configurable), but you get an homogeneous codebase for some little itches you can live with. Then formatting becomes a closed topic, you can move on and focus on the real stuff.

For the formatting, it doesn't happen in the CI. Formatters like black or isort (but not yapf it seems) have a --check switch that can just check that the code is properly formatted. So the CI does no formatting, it just tells you that your code is not properly formatted, and where. It's the user responsability to format the code. To automate these frequent operations, we use invoke, that lets you define tasks like invoke format, invoke lint, invoke test, and that you can chain like invoke format lint test etc. This means the developper and the CI call almost the same invoke tasks, leading to very readable CI scripts with almost no logic, the logic being in python code (invoke tasks are defined in a tasks.py file).

To sum up, our developer workflow is:

• the developper writes code
• they run invoke format, which calls black and isort to format the code
• they run invoke lint which will call flake8, and fix the problems
• they run invoke test, and fix the tests
• they commit and push.

The CI will run invoke check-format instead of invoke-format, which will trigger an error and break the pipeline if the developper didn't format its code, but that's pretty much the only difference.

Now the hooks. We don't use any, but I prefer when each commit is formatted properly, because if you don't, removing a commit or cherry-picking doesn't ensure that the code you get is formatted properly. This hasn't been that much of a problem for now, but pre-commit hooks that perform a check on the formatting would be a solution. They would either remind the developer to run invoke format before committing, or run it for them.

[liberforce]

BTW, I think this "code quality" bug is way too large. Splitting it into multiple bugs on which this one would depend would be much less daunting for newcomers that want to help, and would avoid mixing different topics in the comments, so the conversation is more focused and the decisions clearer. Ideally that would be one bug by checkbox, or at least one per category (organize/modernize/cleanup). This is all stuff that can be done independently.

If you want to get people jump into the boat, having small definite tasks that can be easily done, eaisily reviewed, and thus quickly merged is a must.

[diegogangl]

Now the hooks. We don't use any, but I prefer when each commit is formatted properly, because if you don't, removing a commit or cherry-picking doesn't ensure that the code you get is formatted properly. This hasn't been that much of a problem for now, but pre-commit hooks that perform a check on the formatting would be a solution. They would either remind the developer to run invoke format before committing, or run it for them.

Yep, that's the idea: a pre-commit hook that checks the code. We can trust that every contributor will know about checking/formatting tools and will also run them.

BTW, I think this "code quality" bug is way too large. Splitting it into multiple bugs on which this one would depend would be much less daunting for newcomers that want to help, and would avoid mixing different topics in the comments, so the conversation is more focused and the decisions clearer. Ideally that would be one bug by checkbox, or at least one per category (organize/modernize/cleanup). This is all stuff that can be done independently.

This can't be all done independently. It's kind of sequential actually. Gactions require GtkApplication. Flatpak requires Meson (so we can take advantage of gnome builder and profiles/options). Moving stuff around should be done after resources and Gtkapplication port so we don't have to change more stuff than necessary. Moving the ui files to /data also requires Meson, so it can copy them along with the rest of the code when installing. AFAIK decorators also requires having the ui files in resources first.

It also doesn't make sense to do the cleanup stuff until all the porting/refactoring and moving around is done. I've already changed a ton of things in the GtkApplication branch and still have to move quite a few more.

Doing this independently could also cause a ton of git conflicts.

If you want to get people jump into the boat, having small definite tasks that can be easily done, eaisily reviewed, and thus quickly merged is a must.

There's a bunch of tasks in the low-hanging-fruit label (maybe we could rename it to beginner jobs or something?). I'll add a couple more that I have been thinking about.

[SqAtx]

Hehe, I came here to propose adding pre-commit to the repo, and I see that there is already a whole conversation about it :)

The problem that prompted me is that we have a lot of trailing whitespaces in GTG. Since my editor removes them on file save, I get a lot of noise when trying to make a commit.

So I was thinking of making pre-commit run some simple checks, starting with trailing-whitespace like in this other project I've written.

Then we can iterate from there and add more stuff. I'm most familiar with pylint and yapf, but we can discuss exactly which ones we want.

Doing the cleanup on a branch one-by-one seems like the best way.

I'm not a fan of having a long-lived "cleanup" branch that's separate from master, because it's going to be a pain to rebase all the time, then when we merge it, devs with feature branches will also have to do a potentially painful rebase.

I'd instead propose to add very few checks, for example telling pylint to ignore a bunch of problems. Then un-ignore them and fix them, one at a time.

Then formatting becomes a closed topic, you can move on and focus on the real stuff.

Something to look forward to!