chore(drizzle): remove unused variables #12336
Conversation
jordisala1991
changed the title
jordisala1991
changed the title
| export async function up({ db }: MigrateUpArgs): Promise<void> { | ||
| ${indent(upSQL)} | ||
| } | ||
|
|
||
| export async function down({ db, payload, req }: MigrateDownArgs): Promise<void> { | ||
| export async function down({ db }: MigrateDownArgs): Promise<void> { | ||
| ${indent(downSQL)} |
There was a problem hiding this comment.
db is also not used:
export async function up(): Promise<void>
export async function down(): Promise<void>
There was a problem hiding this comment.
db is used in the generated migration
There was a problem hiding this comment.
Oh, it's a function generator, I see.
I'm not familiar with this part of the code, so at first glance, it's not obvious to me that what indent returns may contain db but not payload or req. Perhaps it depends on the use case and could be a problem for other people, but not for you?
Unless there's a good reason, I prefer to play it safe and leave it as is. It's not a big deal. Codegen will always depend on the user's eslint configuration, and removing parameters, if there are any, is trivial and takes a second.
There was a problem hiding this comment.
Unfortunately this eslint rule is not auto fixable. It must be fixed manually. Are there any case where other variable than db might be used here by default?
If you prefer not to touch it because you are unsure of the edge cases, I am fine with it, but it takes time for each migration you generate (right now I am manually removing them). To me it is a quality of life improvement.
There was a problem hiding this comment.
Are there any case where other variable than db might be used here by default?
That's the key question.
I agree it would be an improvement if this were the case, but the time it would take to investigate whether it's safe should be longer than removing an unused parameter.
If you want to provide an explanation that doesn't require in-depth analysis, I'd be happy to review it. Otherwise, I'd say we close this PR.
In either case, thanks for your feedback and time!
There was a problem hiding this comment.
Let me take a deeper look at the code to see if I can be sure only db will be used
What?
Remove unused parameters on the migration file.
Why?
Because it is reported by eslint as unused parameters
How?
Removing them by default, if someone needs to use them, they can still add them back.