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

Make linting stricter and fix linting #106

Merged
merged 10 commits into from
May 13, 2021
Merged

Make linting stricter and fix linting #106

merged 10 commits into from
May 13, 2021

Conversation

ferferga
Copy link
Member

This PR adds dependabot and extra linting settings to the ones added to #99. They are really similar to the ones enforced in Vue and Web

#99 must go first, as it adds extra packages needed for these settings to work, this PR just adds extra settings that are not related to the TS migration.

@ferferga ferferga requested a review from YouKnowBlom December 31, 2020 12:34
@ferferga ferferga marked this pull request as ready for review January 1, 2021 17:54
@ferferga ferferga removed the blocked label Jan 1, 2021
@ferferga ferferga mentioned this pull request Jan 1, 2021
10 tasks
Copy link
Contributor

@hawken93 hawken93 left a comment

Choose a reason for hiding this comment

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

Looks good, except my annoyance that I'd like the changes to go with the linting so that we're not buried deep in lint warnings.

Even before this there are some 260 linter warnings/errors and this will increase the errors even more. It will decrease the warnings some due to incomplete JSDoc but I'd rather have linter warnings than empty comment blocks. I think it's a bad thing that eslint even autofixes to empty comment blocks and params without types or descriptions, these should all be warnings too, but they are not.

In such a situation, new code may introduce more linter remarks, not less, because the linter loses its functionality as a mechanism to keep the code clean.

Also, if you know any checks to make sure public class members in typescript also have documentation, then I welcome this addition.

I understand that you may not be able to make good jsdoc comments, but in this regard we can prioritize not having any errors, and I'll fix jsdoc that requires knowing the code over time.

src/components/jellyfinActions.ts Outdated Show resolved Hide resolved
src/components/jellyfinActions.ts Show resolved Hide resolved
src/components/jellyfinActions.ts Outdated Show resolved Hide resolved
src/components/maincontroller.ts Show resolved Hide resolved
@ferferga
Copy link
Member Author

@hawken93 will do once we finish all the huge refactoring you did. Didn't want to mess with that until everything was in place 👍🏻.

@ferferga ferferga mentioned this pull request Feb 12, 2021
@ferferga ferferga changed the title Make linting stricter and add dependabot Make linting stricter and fix linting Feb 12, 2021
@ferferga ferferga marked this pull request as draft February 12, 2021 23:53
@ferferga ferferga force-pushed the lint-dependabot branch 2 times, most recently from 073b10a to c0b49e3 Compare February 13, 2021 00:01
@ferferga ferferga mentioned this pull request Apr 12, 2021
@ferferga
Copy link
Member Author

Using all the identation settings from Vue and fixed all the linting errors (warnings from JSDoc are still there)

Most of the remaining earnings can be fixed as soon as we have proper typings with axios client.

Marking as ready to review.

@ferferga ferferga marked this pull request as ready for review April 12, 2021 23:54
@ferferga ferferga requested a review from hawken93 April 12, 2021 23:54
@ferferga ferferga force-pushed the lint-dependabot branch 2 times, most recently from 56b2091 to a272a7c Compare April 13, 2021 00:01
.prettierrc Outdated Show resolved Hide resolved
@ferferga ferferga force-pushed the lint-dependabot branch 2 times, most recently from 1449f2d to a0d4687 Compare April 14, 2021 22:21
.eslintrc.js Outdated Show resolved Hide resolved
@hawken93
Copy link
Contributor

👍

@ferferga ferferga merged commit e450fa8 into master May 13, 2021
@ferferga ferferga deleted the lint-dependabot branch May 13, 2021 11:07
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