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

Fix linter warning #480

Merged
merged 6 commits into from
May 31, 2024
Merged

Fix linter warning #480

merged 6 commits into from
May 31, 2024

Conversation

0x7B5
Copy link
Contributor

@0x7B5 0x7B5 commented May 29, 2024

Fixed all 43 linter warnings.

Fixes #478

@0x7B5 0x7B5 marked this pull request as ready for review May 29, 2024 22:38
@0x7B5 0x7B5 changed the title [WIP] Fix linter warning Fix linter warning May 29, 2024
Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

Our philosophy on linters is that it should mostly be flagging things that are unambiguously bugs or patterns that lead to bugs.

Also, can we turn all warnings into errors so CI catches this in the future?

pnpm-lock.yaml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

the diff is massive due to this. can you revert? do you know if there's some way to not update this on pnpm install?

Copy link
Contributor

Choose a reason for hiding this comment

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

#483 should fix this


const FILE_NAMES = {
qcameras: 'qcamera.ts',
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a large part of the diff, what's the rationale behind it? maybe we should just ignore it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the stylistic changes are from vscode auto-formatting the code, I'm going to revert these changes.

src/actions/files.js Outdated Show resolved Hide resolved
src/actions/files.js Outdated Show resolved Hide resolved
src/actions/files.js Outdated Show resolved Hide resolved
src/actions/files.js Outdated Show resolved Hide resolved
@AkechiShiro
Copy link

Is the PR good enough, there is no need to submit a new one ? I should probably work on something else ?

@0x7B5
Copy link
Contributor Author

0x7B5 commented May 30, 2024

Redid this PR keeping this in mind: "Our philosophy on linters is that it should mostly be flagging things that are unambiguously bugs or patterns that lead to bugs":

  • Changed all linter warnings to errors
  • Fixed code that followed the philosophy listed: no-case-declarations, no-use-before-define, no-empty, etc.
  • Added linter ignore for code that didn't need to be flagged

(I also ignored "class-methods-use-this" and "no-nested-ternary" since those patterns show up a lot in the codebase and don't seem like they match the philosophy. I can change those to warnings or refactor the relevant code if needed.)

@@ -25,7 +25,7 @@ export default function reducer(_state, action) {
let state = { ..._state };
let deviceIndex = null;
switch (action.type) {
case Types.ACTION_STARTUP_DATA:
case Types.ACTION_STARTUP_DATA: {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do these have brackets and the other cases don't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing this warning "Unexpected lexical declaration in case block" it was only for the blocks that had variables within case scope. Should I just ignore that warning or put brackets on the other cases as well?

@adeebshihadeh adeebshihadeh merged commit bd4f6cc into commaai:master May 31, 2024
4 checks passed
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.

Fix all linter warnings
3 participants