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

Code Quality (no-nonsense JS) #2650

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

4yman-0
Copy link

@4yman-0 4yman-0 commented Nov 2, 2024

No description provided.

@4yman-0 4yman-0 marked this pull request as draft November 2, 2024 17:03
@4yman-0
Copy link
Author

4yman-0 commented Nov 2, 2024

A deicision has to be made about features/compatibility.
Use let and const or just var?
Use fancy for of and for in or for (var i = 0;...)?
Use if (!ok) return or (ok) => {if (ok){ ... }}?
Many people have contributed to this project. Without some sort of style guideline or Continous Intergration it had developed into spaghetti try catch timeout try catch code.

@4yman-0 4yman-0 self-assigned this Nov 3, 2024
@4yman-0
Copy link
Author

4yman-0 commented Nov 3, 2024

@ImprovedTube hi!

@ImprovedTube
Copy link
Member

hi again and thanks! @4yman-0 - this must have taken a while!! Makes me hopeful you spend more hours here in future.

Most of the current code was still written and structured by the same author carefully, who is gone since 2022.

Architecture: #2415 (#2459) #2276


Use let and const or just var?

The majority of existing choices (main authors) will be for a reason. The random (some authors) might not matter.
We can use whats best and or shortest and define a guideline.
in 2024 @raszpl edited a lot and prepared/demonstrated much more linting at https://github.com/raszpl/tweaks4ytube
(not sure either if / when @raszpl will be back unfortunately)

into spaghetti try catch timeout try catch code.

Please note down if anything really seems complication, like slowing down development. I might added some quick remedies /tests without indentation or line breaks and (so they can be identified like that too. ) (So that's just work in progress and formatting these equally makes that less clear. Some of it might also be ok to stay for 20 years. who knows, as long as we have open issues and more important tasks than reducing nano seconds of cpu.)

Reducing code size:
RE: aacda82 sometimes the code checks if features are set true in storage, because false values were also stored not deleted (mostly needlessly) and few (seven?) feature are true by default too. Pending PR: https://github.com/code-charity/youtube/pull/2462/files )

Misc: We could shrink the code by some more percent #1634, for example most (all?) of the improvedtube.something() functions checking improvedtube.storage.something etc. could use "this" - if in all foreseeable future the functions need not be called differently changing "this".

@4yman-0
Copy link
Author

4yman-0 commented Nov 4, 2024

into spaghetti try catch timeout try catch code.

Please note down if anything really seems complication, like slowing down development.

I don't really think that slowing down developement. TBH try catch timeout try catch can be formatted nicely and called error handling (for YTs frequent changes). It didn't appear out of thin air.

We can use whats best and or shortest and define a guideline.

here's CanIUse for ES6 (the kinda new javascipt) which the upstream (code-charity) code is already using (const let obj?.something). I'm sure if users really cared there would have been an issue long before this PR. HOWEVER it breaks satus (probably because of () => { this }) so I will not touch it.

Reducing code size:
RE: aacda82 sometimes the code checks if features are set true in storage, because false values were also stored not deleted (mostly needlessly) and few (seven?) feature are true by default too. Pending PR: https://github.com/code-charity/youtube/pull/2462/files )

Misc: We could shrink the code by some more percent #1634, for example most (all?) of the improvedtube.something() functions checking improvedtube.storage.something etc. could use "this" - if in all foreseeable future the functions need not be called differently changing "this".

This PR is called "Code Quality". I can work on a "Use BASH for CI and remove async loading" after this is resolved.

Edit: Add stuff.
Edit 2: See the comment below.
Edit 3: Elaborate on why satus did not work.

@4yman-0

This comment was marked as outdated.

@4yman-0
Copy link
Author

4yman-0 commented Nov 10, 2024

After a lot of trial and error. I will not use ES6 because it breaks satus

@4yman-0 4yman-0 marked this pull request as ready for review November 10, 2024 12:48
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.

2 participants