[legacy-framework] Fix additional lint rules #366
Conversation
packages/cli/src/commands/db.ts
Outdated
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable eqeqeq */ | |||
There was a problem hiding this comment.
it seems == is being used intentionally here over ===, so I'm disabling this rule for this file only. In general, I think this is a rule we want to promote, so I didn't disable it globally
There was a problem hiding this comment.
Hmm. code is a number so to me that looks like it should be referential equality.
There was a problem hiding this comment.
code can actually be null:
https://nodejs.org/api/child_process.html#child_process_event_exit
If the process exited, code is the final exit code of the process, otherwise null
Looking at this,=== would definitely be what we want here, since we are targeting the success case. I'll remove the disable and fix the equality comparison
There was a problem hiding this comment.
Typescript is also inferring it properly as number | null so I'm removing the type declaration on the argument of the exit event
There was a problem hiding this comment.
I love it when linting solves real world bugs! 😀
There was a problem hiding this comment.
I love it when linting solves real world bugs! 😀
@ryardley I saw your tweet before I saw this comment, the tweet makes much more sense now 😂
jportela
force-pushed
the
fix-additional-rules
branch
from
1933d00 to
05e2bae
Compare
| @@ -1,8 +1,7 @@ | |||
| import {Command} from '@oclif/command' | |||
| import {build} from '@blitzjs/server' | |||
|
|
|||
| // eslint-disable-next-line import/no-default-export | |||
There was a problem hiding this comment.
just checking, because of the eslint-disable comments, do we intend to export these as default?
There was a problem hiding this comment.
nope, that's just legacy. moving forward we'd like everything to be named exports
|
Uh oh, lot's of conflicts 😬 |
|
no worries, that's to be expected, should be straightforward to solve. If we plan to merge soon, I can do that now |
|
@jportela yeah I can merge soon |
|
@flybayer how do you feel of upgrading these two linting rules to |
jportela
force-pushed
the
fix-additional-rules
branch
from
05e2bae to
5bde8dd
Compare
|
@jportela which rules? |
|
the |
|
I.e. the current code is all correct and that we can now change those to be an error to ensure new PRs conform? If this is what you are asking, then yes let's error them. |
jportela
force-pushed
the
fix-additional-rules
branch
from
5bde8dd to
4810d61
Compare
|
@jportela Good to merge? |
|
@flybayer yes, just need to wait for the build checks to finish :) |
|
@flybayer I actually found a typo on my .eslintrc changes, I'm pushing the fix for it now |
jportela
force-pushed
the
fix-additional-rules
branch
from
4810d61 to
415db25
Compare
|
@flybayer done, feel free to merge whenever 👍 |
|
Sweeet, thank you! |
itsdillon
changed the title
Type: linting
Closes: blitz-js/legacy-framework#803
Follow-up to: #298
What are the changes and their implications? ⚙️
Uniform linting rules across all subpackages. 0 warnings, 0 errors.
Checklist
packages/server/src/log.tsBreaking change: no
Other information