-
Notifications
You must be signed in to change notification settings - Fork 662
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
chore: tweak lint config to allow for eslint --fix
to fix import order
#1827
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1827 +/- ##
==========================================
+ Coverage 81.81% 81.85% +0.04%
==========================================
Files 35 35
Lines 7763 7782 +19
Branches 317 317
==========================================
+ Hits 6351 6370 +19
Misses 1400 1400
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome PR 😭 The mental friction of "where do I put this import" shall no longer be. And this just looks a lot better. Thank you!
Left one comment with confusion on import ordering and one on comments, but neither are blocking! 🚀
import stream from 'stream'; | ||
|
||
import { assert } from 'chai'; | ||
import sinon from 'sinon'; | ||
|
||
import { shell } from './shell'; | ||
|
||
import type { ShellProcess } from '../utils/types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😱 Big fan of splitting stdlib from dependencies from local from types if that's what this rule is requiring!
@@ -1,4 +1,4 @@ | |||
import { createLogger, format, transports, Logger } from 'winston'; | |||
import { Logger, createLogger, format, transports } from 'winston'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uppercase alphabetization is a big winston 🏆
packages/web-api/src/methods.ts
Outdated
// Request types | ||
import type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗒️ Might be nice to also swap 🪓 the request
and response
comments for these moving types!
LinkUnfurls, | ||
MessageAttachment, | ||
MessageMetadata, | ||
} from '@slack/types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ordering seems unexpected to me - I'd've thought @slack/types
finds a place before ./*
imports - but no blocker if this is what the linter likes 🙏
Also only noticed this in a few web-api
files... Not sure why though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure why.... but I almost don't want to ask questions. Just tell me how to order the damn things and don't make me think!
ac6c902
to
8ec1793
Compare
Small maintainer QoL fix: tweaked the eslint config to use a combination of ESLint's stock
sort-imports
rule andeslint-plugin-import
'simport/order
rule to allow foreslint --fix
to fix import ordering. This was driving me a bit crazy eyetwitch and I found this comment that showed off using the two rules in combo.Also ensured all the
lint
npm run scripts use the--fix
rule across all monorepo packages.The only change worth reviewing is within
lint-configs/
- all other file changes are simply style fixes.