floralvikings / jira-connector

NodeJS Wrapper for the Jira REST API
http://floralvikings.github.io/jira-connector/
MIT License
373 stars 180 forks source link

Add & apply linting #208

Closed G-Rath closed 4 years ago

G-Rath commented 4 years ago

🎉

I'm not a contribue on the repo, so can't setup any pipelines or actions, but here you go!

I used my standard configuration with minimal changes so you can see what it's like.

In particular, it does mean it's using 2 spaces instead of 4. I've also committed the package-lock.json, as that's good practice.

Happy to discuss and explain anything if you've got questions, or want to request changes :)

I am interested in knowing what language level is being targeted, as it looks to be pre-ES6, but if we're happy w/ ES6+ we can clean up quite a bit w/ template strings & default properties 🤷‍♀


Also: I deprecated path_prefix in the config, since it was easily done. Everything seems to typically be in camelCase; for now I've added the other nested snake_case properties to the ignore list of the eslint rule, and will circle back once things have stabilized to deprecate them.

G-Rath commented 4 years ago

I've chucked on checkJs too, w/ // @ts-nocheck now on every js file, as the majority of api files are looking pretty decent, w/ no funky weird errors - it's just general JSDoc improvements, so I think it'll be worth it given that // @ts-nocheck means we can incrementally fix up each js file at a time :D

It has highlighted however that the existing TypeScript definitions are slightly wrong, as they're not correctly representing how each class is exported; it should be:

export = class ...

This means that you can't export anything else in the definition unless it's in a namespace a la how JiraClient is exported. Again this is something we can take care of incrementally, and ideally nothing should be exported from the api classes aside from the classes themselves. (they can have other definitions, but they should only being consumed by themselves - anything public/exported should live in index.d.ts).

Also, I found a let in JiraClient, so I guess we're ES6 🎉

MrRefactoring commented 4 years ago

This is a really huge PR. It takes me a while to think and analyze what we can do. I am afraid that we cannot switch to es6 so easily

G-Rath commented 4 years ago

@MrRefactoring Completely understandable; just so we're clear:


  • I've not used any ES6 features,

Ah but eslint --fix has used let instead of var - I can revert that, but like I said let is actually already being used in at least one place.

MrRefactoring commented 4 years ago

Oh my

It seems you're right)

Added 3 years ago

G-Rath commented 4 years ago

I just realised, promises are ES6 too :joy: