Linting Codebase

[derenv]

It would be good to clean up the codebase, there's a few tasks to do for this:

• [x] Remove any remaining invalid imports
• [x] Agree on linting scheme
• [x] Implement linting scheme
• [x] consistently use proper operators (=== instead of ==, etc)
• [x] remove unused variables and constructors or mark them as unused or exported if intended
• [x] replace string concatenation with format strings
• [x] Decide on comment schema for file/method/class
• [x] Implement comment schema
• [x] Decide on in-file license format
• [x] Implement in-file license format

EDIT: updating this list as needed

[kenohassler]

Yes, I agree! I think we should add an .eslintrc file, so that all contributors automatically use the same linting style. As a starting point, I used the ESLint configuration from gnome-shell, commented out the jsdoc rules and ran it on the project. After whitespace linting, there are 77 problems remaining, mostly one of:

• consistently use === instead of ==
• remove unused variables and constructors or mark them as unused if intended
• replace string concatenation with format strings

In a second step, we could also discuss a commenting style and perhaps use linting rules to check conformance.

[derenv]

This seems like a good method!

I think we should add an .eslintrc file, so that all contributors automatically use the same linting style. As a starting point, I used the ESLint configuration from gnome-shell, commented out the jsdoc rules and ran it on the project.

Are you able to push this to the ethanwharris/gnome-nvidia-extension/linting_cleanup branch? or is a pull request required?

[derenv]

In a second step, we could also discuss a commenting style and perhaps use linting rules to check conformance.

As for commenting style, yes i think adding a subset of linting rules for this is a good idea. Ideally we should create a github action that runs the linting rules when we try to pull request.

I personally prefer this for methods/classes:

/*
 * name: method/class name
 *
 * last modified: user who modified and date of modification
 *
 * description: short concise description of purpose
 */

with a similar layout using " * " at the start of lines for large multiline comments.

[derenv]

Useful documentation..

[kenohassler]

Are you able to push this to the ethanwharris/gnome-nvidia-extension/linting_cleanup branch? or is a pull request required?

I need to create a pull request. In the meantime, have a look at my fork for a first impression of the linting result :slightly_smiling_face:

As for commenting style, yes i think adding a subset of linting rules for this is a good idea. Ideally we should create a github action that runs the linting rules when we try to pull request.

Good idea! I'll have a look into it.

Useful documentation..

I can recommend the GJS Styleguide and their linting recommendations, which include the gnome-shell .eslintrc I used as well as the GJS .eslintrc.

BTW: The linter found something I didn't understand - I'm not exactly a GObject expert though: What does this undefined(?) underscore do here?

[kenohassler]

I think this ticks all the boxes not related to comments. For comments, I'll have a look into jsdoc. The scheme you proposed looks good to me, though I'm not sure about the last modified info. Anyway, we might want to restrict comments to the functions and classes exported from a file for now cause even this is a LOT of writing to do :joy:

One more thing - I don't know if you like the idea. We could perhaps save some space removing the redundant license headers in every file and replacing them with two-line SPDX identifiers (examples in .eslintrc and subprocess.js, for the general idea see https://reuse.software/).

[derenv]

I need to create a pull request.

Ah, thanks! i wasn't sure if it was only main branch that was push protected.

In the meantime, have a look at my fork for a first impression of the linting result

sure, i'll review it now.

Good idea! I'll have a look into it.

I've made similar stuff for Rust CI, see here for the file i made - it shouldn't be too difficult, just a case of adapting the 'lints' action from there to use eslint with our source. The basic yml structure isn't too complicated.

I can recommend the GJS Styleguide and their linting recommendations, which include the gnome-shell .eslintrc I used as well as the GJS .eslintrc.

thanks, these are helpful!

BTW: The linter found something I didn't understand - I'm not exactly a GObject expert though: What does this undefined(?) underscore do here?

I'm really not sure, i spotted it when i was messing about with the gnome-shell eslintrc (i spent a bit of time getting eslint configured with vscode) - can you try replace:

super._init(0.0, _("GPU Statistics"));

with

super._init(0.0, "GPU Statistics");

and see if it runs properly? I switched to plasma a while ago so you're probably going to be faster/better at testing this than i will be in a VM.

I think this ticks all the boxes not related to comments. For comments, I'll have a look into jsdoc.

sure, let me know what you find.

The scheme you proposed looks good to me, though I'm not sure about the last modified info. Anyway, we might want to restrict comments to the functions and classes exported from a file for now cause even this is a LOT of writing to do

Ha, we can drop that: its a habit i picked up when working on a codebase that didn't use git, was around 20 years old, and was modified by various people who more often than not weren't involved anymore - it helped keep me sane! but yeah, restricting comments to (edit)exported(edit) functions and classes just now seems fine.

One more thing - I don't know if you like the idea. We could perhaps save some space removing the redundant license headers in every file and replacing them with two-line SPDX identifiers (examples in .eslintrc and subprocess.js, for the general idea see https://reuse.software/).

okay, that looks interesting! i'll read up on that at some point soon.

[derenv]

@kenohassler I've sorted through the spdx licensing in #193 and managed to only give myself a slight headache!

[kenohassler]

I added a few nitpicks in the PR. Can fix them if you want. Great job on REUSE compatibility! 👍

BTW: Did the linting check work for you? Strangely, I don't see an ESLint result in the PR. Could be because the github action is not yet in master, or because there simply are no linting errors, or maybe I messed up the CI config? Let's keep an eye on this. (Edit: same goes for the new REUSE action. Probably not my issue then, perhaps something about branch protection?)

[kenohassler]

I added a PR addressing the issues I mentioned, plus a handy github action to check for REUSE compliance.

[derenv]

Did the linting check work for you?

i think so! im not sure about the PR i made, i think the best way to sort this is to ask @ethanwharris to give you permissions and require reviews before merges for everyone so we can properly review PRs.

it's definitely working for your more recent PRs:

[ethanwharris]

Added @kenohassler as a collaborator 🎉 Thanks for your contributions!