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

Introduce code linting #1024

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

Conversation

expelledboy
Copy link

@expelledboy expelledboy commented Nov 10, 2020

So just to clarify, its not about forcing a coding standard down on you ~ #227

To justify why I feel linting would assist this project:

  • Linting gives you suggestions while you are coding to identify issues
  • Best practices actually improve not only security but performance in some cases
  • Catches common errors that sometimes take ages to find, due to a missing comma/bracket/etc

But most important, so I will stipulate this separately. Linting is essential for code review!

Also this project has files which do not parse as valid JavaScript, currently npm run lint
actually returns errors that have nothing to do with styling.

And you will find this is the most forgiving/allowing code linting rules in any project.

I have actually disabled the majority of the linting rules so that you can opt-in rather
than deal with the massive task of bringing this to up to date with best practices.

Also I see you have massive branches you need to merge so...

That being said I feel there are some rules which are disabled that as I said are essential imho

I am happy to take a phased approach at this point, without linting I feel like I am
walking drunk into an African brothel without a condom 🤪

This will effect your development workflow, with the addition of the pre-commit and
pre-push git hooks. You should enable editorconfig and eslint plugins in your IDE
to guide you to not have issues when it comes time to commit or push.

@sirpy
Copy link
Collaborator

sirpy commented Nov 11, 2020

@expelledboy no doubt this is required:)
maybe also push the files after lint, otherwise people will start having merge conflicts if we in did start using this

@expelledboy
Copy link
Author

So I can introduce this as both a pre-commit hook and into npm test, but could I get assistance with some of the errors of npm lint?

I dont want to make too many changes to introduce linting, and I am not confident yet with the codebase.

@expelledboy
Copy link
Author

Okay so I disabled even more checks 🤣 well yolo, we can bring in more rules as and went we don't have large branches to merge.

@expelledboy
Copy link
Author

Nice CI working with npm lint

https://travis-ci.org/github/amark/gun/builds/742923538#L230

@expelledboy
Copy link
Author

expelledboy commented Nov 11, 2020

Am I pushing it too far to also add code coverage limit of 65% while I am at it? :)

You guys actually have done a solid job so far! I wasnt expecting to set to all the way up
to 65% when the project didnt actually include coverage reports.

---------------|---------|----------|---------|---------|---------------------------------------------------------------
File           | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
---------------|---------|----------|---------|---------|---------------------------------------------------------------
All files      |   65.75 |    56.94 |   64.55 |   69.32 |
 gun           |   64.62 |    56.92 |   63.43 |   67.69 |
  gun.js       |   66.78 |    59.35 |    63.5 |   69.59 | ...870-1871,1946-2084,2098-2361,2368-2377,2382-2386,2412-2450
  sea.js       |    60.4 |    51.29 |   63.29 |   64.29 | ...221,1231,1234,1237-1239,1243-1245,1250,1255,1280,1290-1438
 gun/lib       |   69.76 |    57.01 |   69.23 |   75.14 |
  fsrm.js      |   84.62 |    66.67 |     100 |   91.67 | 12
  radisk.js    |   67.64 |    53.65 |   75.81 |   73.18 | ...01-426,429-437,474-475,479-481,489,512-531,536-560,566-568
  radix.js     |   91.07 |    89.34 |      75 |   94.94 | 93-94,108-109
  radmigtmp.js |      45 |       25 |      50 |      50 | 6-8,11,15-17,20
  rfs.js       |   73.68 |    61.29 |      75 |   78.85 | 33,44-57,68
  serve.js     |    10.2 |        0 |       0 |   13.89 | 7-47
  store.js     |   80.75 |    58.22 |   85.71 |   89.42 | 38-43,132-136
---------------|---------|----------|---------|---------|---------------------------------------------------------------

Another question is would you like it to run a full npm test on a push? You can easily skip it
you are confident that your changes dont break anything by running git push --no-verify

@expelledboy
Copy link
Author

I dont know how this is merge conflicting, I literally copy the file over mine from master...
git show remotes/origin/master:lib/wave.js > lib/wave.js

/gun.ts
/temp/
.nyc_output/
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This btw is a prefect example of why you need linting. This file clearly had CRLF line breaks, and because I added 1 small change at the end my editor detected inconsistencies and normalized it. The .eslintignore file actually has a list of such files in the *.js realm which fail to be consistent with the rest of the project.

@noctisatrae
Copy link
Contributor

It'd be excting for code quality!

@amark
Copy link
Owner

amark commented Jul 13, 2022

My life was hell in 2020, and I'm still dealing with the aftermath. Saw this because of noctis's comment.

The code coverage parts sound really cool (I never knew how to do those).

The linting part, as you know, I don't like. A few of the rules you picked out tho I'm in favor of, but given how much hell, stress, depression, etc. I'm recovering from, and impossible deadlines in multiple areas of my life, this is a "let's try it after mymore basic goals have been completed/finished."

Is there a way to get the code coverage without linting? Thanks!

@expelledboy
Copy link
Author

@amark I feel like we are in a similar boat. Been overworked since the last time we chatted in Discord, which was when I created this.

As requested I broke out the code coverage, and editor config files, into separate PRs that the maintainers can comment and amend. 👍🏻

That being said I put quite alot of time into getting an eslint config that actually works with this codebase, so I hope that you will reconsider. Auto formatting and the errors in the IDE are a massive productivity boost, and to be honest quite integral to my dev workflow.

@amark
Copy link
Owner

amark commented Sep 27, 2022

Sorry to hear that :( as well.

Been gone at bunch of conferences this last month, sorry for the delay.

Thanks for splitting things up. Which / how / teach me the parts on the code coverage?

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.

4 participants