-
Notifications
You must be signed in to change notification settings - Fork 38
Handle 'true'/'false' stringified boolean options and better config error handling #520
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
Conversation
8a1acfd
to
8949b37
Compare
Coerce 'true'/'false' into actual booleans (Fixes ember-codemods#223) Better error messages
@@ -20,7 +20,7 @@ const transformer: Transform = function ( | |||
|
|||
if (replaced) { | |||
source = root.toSource({ | |||
quote: userOptions.quotes ?? userOptions.quote, |
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.
quotes
was never documented and was redundant with quote
, so I removed it.
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.
Looks good! I have a few (non-blocking) questions, mostly because I'm new to zod.
@@ -0,0 +1,254 @@ | |||
import { describe, expect, test } from '@jest/globals'; |
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 loving all these new tests. Great job!
transforms/helpers/options.ts
Outdated
return typeof arg === 'string' ? arg.split(/\s*,\s*/) : arg; | ||
}, z.array(z.string())), | ||
inObjectLiterals: StringArraySchema.describe( | ||
'Allow-list for decorators currently applied to object literal properties that can be safely applied to class properties.' |
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 find this message pretty confusing. What exactly is it trying to say?
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.
Input:
Lines 3 to 6 in f8d8fd2
const Foo = EmberObject.extend({ | |
@userAdded | |
whatever: undefined | |
}); |
Config:
Lines 1 to 5 in f8d8fd2
{ | |
"decorators": { | |
"inObjectLiterals": ["userAdded"] | |
} | |
} |
Output:
Lines 4 to 8 in f8d8fd2
@classic | |
class Foo extends EmberObject { | |
@userAdded | |
whatever; | |
} |
Without the config, the @userAdded
decorator would not transform at all because it's applied to a property and not a method.
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.
What about something like:
A list of decorators that are allowed in object literals.
When the codemod finds a field with one of these decorators,
it will be translated directly into a class field with the
same decorator. Including a decorator in this list means
that you believe that the decorator will work correctly
on a class field.
@@ -1,49 +1,98 @@ | |||
import { inspect } from 'node:util'; | |||
import type { ZodError } from 'zod'; |
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.
TIL zod. What do you think about ts-to-zod?
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 haven't tried ts-to-zod, but zod is already based on TS APIs and terminology, so it's super intuitive (to me) as-is.
import type { ZodEffects, ZodTypeAny } from 'zod'; | ||
import { z } from 'zod'; | ||
|
||
const preprocessStringToBoolean = <I extends ZodTypeAny>( |
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 file seems like a good place to DRY up some of the union error message duplication.
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.
Based on your previous answer, it seems like you considered this but it wasn't worth it. Right?
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.
Correct. I tried it but didn't like the built-in error messages.
'true'
/'false'
into actual booleans (Fixes Passing boolean flag options not working correctly #223)Ideally, we'd use
yargs
for this, but doing so would involve changingcodemod-cli
to allow consumers to pass inyargs
options here:https://github.com/rwjblue/codemod-cli/blob/4a126ec9cd0ff4a482a69aa7f872ec629e23ba66/src/options-support.js#L14