Lint assets too #25
Lint assets too #25
Conversation
|
@emedvedev I've disabled most checks for assets so that we tackle them in batches. Can you have a look at the current errors? Remember you can push to my branch if needed :) |
| function submitForm (ev){ | ||
| ev && ev.preventDefault() | ||
| function submitForm(ev) { | ||
| if (ev) ev.preventDefault() |
There was a problem hiding this comment.
I'm not sure if this is needed. When can this be undefined or null?
|
This pull request fixes 3 alerts when merging 422c71b into c6b8fc4 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 3 alerts when merging 5196fdc into a65794e - view on LGTM.com fixed alerts:
|
|
@emedvedev can you please disable the PR comments on lgtm.com dashboard? |
|
Done, sorry :) |
|
I'll take a look at the errors a bit later today, should be able to merge by EOD. |
|
Not entirely sure why the |
|
It's an issue with GH Actions which I assume it will be solved automatically when people at GitHub notice the issue, if they haven't already. |
| @@ -682,7 +682,6 @@ | |||
| */ | |||
|
|
|||
| function Response(req, options) { | |||
| options = options || {} | |||
There was a problem hiding this comment.
Are we sure this wasn't supposed to be used? Like this.options?
There was a problem hiding this comment.
It's not passed downstream and there's no reference to it anywhere else, so I think it should be safe.
|
GitHub tests pass now, and the LGTM issues in assets are fixed as well, so we can merge this. I'll create a separate PR for the LGTM issues outside of assets. |
|
I actually want to reduce the suppressed rules too eventually :) |
|
Ah! Sorry for merging too soon then, haha. Feel free to open another PR for tackling the suppressed rules :) |
This is totally WIP. I'm just trying to get the most important things out.
I'll rebase when the other PRs are merged.
Best reviewed with the non-whitespace diff: https://github.com/emedvedev/slackin-extended/pull/25/files?w=1