search

ipatalas / vscode-sprint-planner

Plan your sprints in VSCode directly!
MIT License
37 stars 22 forks source link

migrate from tslint to eslint #18

Closed sammeel closed 3 years ago

sammeel commented 3 years ago

Hi

I was jumping into your code to see if I can add the "bug" type, As we are using bugs like we have PBIs. But since I've never actually done a public PR, I thought about doing this one first as vscode started to give warnings on every file I opened.

Nice extension btw. Been using it for about 2 sprints now, and just loving the faster creation of tasks. i've been promoting it in our company as well.

ipatalas commented 3 years ago

Nice, thanks! I'll have a look at this in the following days.

Nice extension btw. Been using it for about 2 sprints now, and just loving the faster creation of tasks. i've been promoting it in our company as well.

Thanks! Appreciate that. Unfortunately I'm in projects managed in JIRA lately so the development of this extension pretty much stopped. I'm open to PRs though. I'll manage to find time for review and to release a new version if needed. I really wish I had more time for this because I still have ideas for improvements and there are a couple of meaningful issues as well but life is life...

sammeel commented 3 years ago

I've updated the eslint rule to use the recommended rules as stated in the .eslintrc.js file (https://github.com/typescript-eslint/tslint-to-eslint-config/blob/master/docs/FAQs.md) Before I had an automtic conversion file to make sure it did not diverge to much from the rules you already had used.

I had some more fixes to do this time. Let me know if you see any issues in the fixes I did.

what I also noticed is the mix of spaces and tabs. Perhaps we should add an .editorconfig?

ipatalas commented 3 years ago

I've updated the eslint rule to use the recommended rules as stated in the .eslintrc.js file (https://github.com/typescript-eslint/tslint-to-eslint-config/blob/master/docs/FAQs.md) Before I had an automtic conversion file to make sure it did not diverge to much from the rules you already had used.

I had some more fixes to do this time. Let me know if you see any issues in the fixes I did.

Cool. Good job here. I have added few comments but again nothing major.

what I also noticed is the mix of spaces and tabs. Perhaps we should add an .editorconfig?

Good point. You're right. That could be useful. I don't have strong opinion on either of the options but spaces seems to be easier to format correctly for multi-line fluent API based expressions for instance.

sammeel commented 3 years ago

I've added the .editorconfig

Your other review comments are also fixed

ipatalas commented 3 years ago

Looks good so far. I will have a final look from within VS Code to see how it works (ESLint) later and I'll approve that later on.

ipatalas commented 3 years ago

Ok, had a look at this in VSCode finally. I had some issues making it work at the beginning but I figured it out. I've let myself change ESLint configuration from JS to JSON. Why? Well.. not sure if it's only for me but I had no intellisense for the rules in .eslintrc.js file. I had to remember/copy-paste everything. When it's in JSON it works much better: image

That reason alone is good enough to have it in JSON for me. What do you think?

I've also added few rules which were there in original tslint.json file because the recommended settings which are inherited don't cover them. That led me to fixing few more issues but now there are no errors.

I'm gonna leave it for couple of days in case you have some comments. If you don't then I'll merge it in.

sammeel commented 3 years ago

I agree with you about the json and intellisense.

So for me this one is good as well.