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

Eslint #2446

Merged
merged 17 commits into from
Jul 4, 2024
Merged

Eslint #2446

merged 17 commits into from
Jul 4, 2024

Conversation

dodieboy
Copy link
Member

@dodieboy dodieboy commented Jul 4, 2024

Updated eslint rule
Move the test file out from tests folder
Fixed node.js workflow error
Fixed more code indent

@dodieboy
Copy link
Member Author

dodieboy commented Jul 4, 2024

@ImprovedTube I move the test file out of tests folder as node.js package can only access file and child file only, if put in tests folder, it cannot test file from e.x. js&css folder

@raszpl
Copy link
Contributor

raszpl commented Jul 4, 2024

e353e2b
if (r.player_size == 'full_window' || 'fit_to_window') {

this is beautiful! It flagged real hard to spot on first glance error

@raszpl
Copy link
Contributor

raszpl commented Jul 4, 2024

those are nice too

/*eslint keyword-spacing: ["warn", { "before": true, "after": true }]*/
/*eslint space-before-function-paren: "warn"*/

this https://github.com/SonarSource/eslint-plugin-sonarjs set of eslint plugins has some nice rules, for example
https://github.com/SonarSource/eslint-plugin-sonarjs/blob/master/docs/rules/no-same-line-conditional.md
would flag

ImprovedTube.commentsSidebarSimple = function() { if(ImprovedTube.storage.comments_sidebar_simple === true){

ImprovedTube.transcript = function (el){ if (ImprovedTube.storage.transcript === true){

ImprovedTube.chapters = function (el){ if (ImprovedTube.storage.chapters === true){

ImprovedTube.commentsSidebar = function() { if(ImprovedTube.storage.comments_sidebar === true){

still cant find anything for flagging lack of space between condition and { if (blabla){ hmm maybe https://eslint.style/rules/js/space-before-blocks thats another plugin :(

@dodieboy
Copy link
Member Author

dodieboy commented Jul 4, 2024

current planning to add this: https://www.npmjs.com/package/eslint-plugin-compat
which check browser compatibility

@dodieboy
Copy link
Member Author

dodieboy commented Jul 4, 2024

those are nice too

/*eslint keyword-spacing: ["warn", { "before": true, "after": true }]*/
/*eslint space-before-function-paren: "warn"*/

this https://github.com/SonarSource/eslint-plugin-sonarjs set of eslint plugins has some nice rules, for example https://github.com/SonarSource/eslint-plugin-sonarjs/blob/master/docs/rules/no-same-line-conditional.md would flag

ImprovedTube.commentsSidebarSimple = function() { if(ImprovedTube.storage.comments_sidebar_simple === true){

ImprovedTube.transcript = function (el){ if (ImprovedTube.storage.transcript === true){

ImprovedTube.chapters = function (el){ if (ImprovedTube.storage.chapters === true){

ImprovedTube.commentsSidebar = function() { if(ImprovedTube.storage.comments_sidebar === true){

still cant find anything for flagging lack of space between condition and { if (blabla){ hmm maybe https://eslint.style/rules/js/space-before-blocks thats another plugin :(

I have tried no-same-line-conditional rule, it is not working for me, not flagging anything. I think what you want is the brace-style rule right, will be adding that in

@raszpl
Copy link
Contributor

raszpl commented Jul 4, 2024

I have tried no-same-line-conditional rule, it is not working for me, not flagging anything.

requires eslint-plugin-sonarjs

brace-style: "error"*/ might work, but it will complain about single line conditionals like
if (!(video && player)) {return;}
on the other hand this might be good as
if (!(video && player)) return;
looks cleaner anyway :)

all those eslint rules not only make code more readable, but they already found few real errors, this is great 👍

@raszpl
Copy link
Contributor

raszpl commented Jul 4, 2024

whoa thats an ugly one

if (object = object[property]) {
and ancient too

@ImprovedTube ImprovedTube merged commit d1383bb into code-charity:master Jul 4, 2024
1 check passed
@ImprovedTube
Copy link
Member

thank you! @dodieboy

"lint": "eslint --ignore-path tests/.gitignore"

@raszpl
Copy link
Contributor

raszpl commented Jul 4, 2024

Yeah, fantastic job @dodieboy.

@ImprovedTube
Copy link
Member

brace-style rule

oh, merged early

Can we undo / adjust the last commit? - Precisely: Let's not add much white space automatically, where it loses format intended by an author (one-liners) (and semantic indentation, which can rather be replaced /enhanced by comments.)
(For a lot of lines it is likely that nobody ever will edit them and thus nobody needs them to fill screens either.)
(And the original author might also become estranged and not recognize the code when it is without that structure and 6 times more lines)

Example:

  • While coincidentally looking at the following bit, can you try to read & understand all?:
    (How many seconds does it take and how many eye-movements?)

	try {
		chrome.runtime.sendMessage({action: 'play'})
	} catch (error) {
		console.log(error); setTimeout(function () {
			try {
				chrome.runtime.sendMessage({action: 'play'}, function (response) {
					console.log(response)
				} );
			} catch { }
		}, 321)
	}

...isn't it still laborious to actually scan mostly empty surfaces? @dodieboy @raszpl /am i the only one still frustrated by this inefficiency?
One might just not enjoys to spend more than a few seconds with easy code, but might thinks:
"Why am i seeing this for no contextual or semantic reason?"
"Why do i belittle / "plague" my brain once again looking at this triangle shaped nothing? My brain is not a punched card machine."
- One might also be tired / stubborn & start to enjoy to operate in "almost punch card machine mode". It might be "meditative", to act for a moment like my purpose is nothing but syntax & indentation. But while technology progresses, why would we push that in other people (if when we can raise productive and creativity beyond that.)

VS:

try {chrome.runtime.sendMessage({action: 'play'})}  // else log the error, then repeat & log the 2nd response too.     
            	catch (error) {console.log(error); setTimeout(function () { try { chrome.runtime.sendMessage({action: 'play'}, function (response) { console.log(response) } ); } catch { } }, 321) }

// Story: this single function-call (chrome.runtime.sendMessage()) unfortunately became two lines long, to debug. This 2nd line can be forgotten & doesn't need human attention, until somebody comes there as of logs in the console, which might or might not keep happening at all - or might just start happening again in future.

Once code is well-predicted not to change, then format can be condensed. (Once syntax is verified, final, transactional.) Since any "density" might reduces under-challenge, increase efficiency. (And for who actually spots a long line, and also is "annoyed" by it, then this attention for this detail is more of a meaningful choice (Unlike long documents for everyone) (and likely developer who cares also has style / beautification in their IDE))

  • So that the remaining indentation is semantic, not format, just like a comment (as said) Expressing the low/subordinate relevance. While losing this semantic information would be a waste, or can be replaced by a comment.

+ Generally on Indentation & braces:

Humans actually need not to see (nor input) both. (One is enough.)

  • Who is used/fast with a certain format could use a temporary beautification at home only (IDE's should offer that just like https://github.com/google/blockly
    • Non-functional characters (space, tab, linebreak) didn't ask to in a repo (aside from python), but fleeting style in a view in an IDE) They are good for comments or convenience. Of course some are even statistically convincing for many people, yet they are personal preferences and didn't asked to be imposed on every developer and didn't required to to be anymore than visual but consume a lot of storage IO in the world.
      • This origins in times when many common views on code didn't even provide color markup.

@raszpl
Copy link
Contributor

raszpl commented Jul 5, 2024

brace-style rule
Let's not add much white space automatically

white space is good for readability

, where it loses format intended by an author (one-liners)

bad format leading to errors, brace-style rule will guard against horrible crimes like this:

		if (navigator.userAgent.indexOf("Firefox") != -1) {
			chrome.storage.local.set({below_player_pip: false})}

I cant see on first glance whats going on. This is what brace-style rule will let us spot and fix.
But I agree it is too aggressive and excessive, this change is wasteful:

			}).catch(function () { getLocale('en', callback); });
			}).catch(function () {
				getLocale('en', callback);
			});

This invocation will allow one liners in braces:
/eslint brace-style: ["error", "1tbs", { "allowSingleLine": true }]/

============================================================

(and semantic indentation, which can rather be replaced /enhanced by comments.)

cant really fix bad formatting with comments :)

Example:

  • While coincidentally looking at the following bit, can you try to read & understand all?:
    (How many seconds does it take and how many eye-movements?)

its perfectly readable :] Seriously, what do you find offensive in this piece of code? other than 'repeating identical behavior and expecting a different result.' aka definition of insanity :]

...isn't it still laborious to actually scan mostly empty surfaces?

no, empty surface is what lets you read if faster because white space also conveys meaning. What is not there is just as important as what is there :) In a tight jumbled up version one has to actually read everything, in properly formatted version you can see by the indentation what is what without reading most of it.

@dodieboy @raszpl /am i the only one still frustrated by this inefficiency

How long and how much have you programmed? Appreciation for code readability comes with experience and age.

? One might just not enjoys to spend more than a few seconds with easy code, but might thinks: "Why am i seeing this for no contextual or semantic reason?" "Why do i belittle / "plague" my brain once again looking at this triangle shaped nothing?

its information, lack of appreciation for it leads to things like this b24ede7 :(
Cramming as much as possible as close as possible leads to bugs.

It might be "meditative", to act for a moment like my purpose is nothing but syntax & indentation.

the purpose is not writing bugs :) There are tons of programmers, mainly C ones (hehe), with hubris believing they are above all that and dont need any help writing error free code. If that was the case we wouldnt need MISRA, Ada, etc.

VS:

try {chrome.runtime.sendMessage({action: 'play'})}  // else log the error, then repeat & log the 2nd response too.     
            	catch (error) {console.log(error); setTimeout(function () { try { chrome.runtime.sendMessage({action: 'play'}, function (response) { console.log(response) } ); } catch { } }, 321) }

seriously cant know what this "code vomit" does without carefully reading it and painstakingly tracing and parsing in my brain all code blocks There are two try and catch all in one group, which one does what, and why? With proper indentation those things dont require any thinking because blocks are aligned according to their roles and order.
EDIT: LOL, only when proofreading my reply I noticed its the same code :D jumbling things together is that bad I didnt realize it on first glance.

Since any "density" might reduces under-challenge, increase efficiency.

If that was really the case we wouldnt be finding and fixing errors after ESLint flagged them in this 3?5? year old project? :)

+ Generally on Indentation & braces:

Humans actually need not to see (nor input) both. (One is enough.)

leads directly to this b24ede7
If what you think is true was true this bug couldnt happen. But its not true, humans need all the help they can get from computers, otherwise we would all read and write directly in binary beep boop :]

@raszpl
Copy link
Contributor

raszpl commented Jul 5, 2024

PS: crowdin.yml is spamming errors
https://github.com/code-charity/youtube/actions/workflows/crowdin.yml

Error
No event triggers defined in `on`

@dodieboy
Copy link
Member Author

dodieboy commented Jul 5, 2024

@ImprovedTube why crowdin.yml is in the workflow folder?

@raszpl
Copy link
Contributor

raszpl commented Jul 5, 2024

Oh sorry dodieboy , I assumed it was somehow linked to elsing install

@ImprovedTube
Copy link
Member

ImprovedTube commented Jul 6, 2024

😀 hi @raszpl
braces, indentation, blockly, etc. are all the same.
just different styles / themes of the diagram of logic loops.
(Each type does the same. Each is enough alone. No information is added or lost adding or removing redundancy. )

So this needn't be a difference between programming languages in the first place, let alone a global convention inflating source code files. And if it could just be a simple style declaration, for how one views code personally at home, that will be the fix.

While writing code I enjoy the fingers recovering from using space tab & enter. But if we enter some white space for semantic reason, this needn't be lost next time somebody clicks 're-format' in some way.

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.

3 participants