Skip to content

Conversation

gitKrystan
Copy link
Collaborator

@gitKrystan gitKrystan commented Mar 2, 2023

Ideally, we'd use yargs for this, but doing so would involve changing codemod-cli to allow consumers to pass in yargs options here:

https://github.com/rwjblue/codemod-cli/blob/4a126ec9cd0ff4a482a69aa7f872ec629e23ba66/src/options-support.js#L14

@gitKrystan gitKrystan force-pushed the better-args branch 2 times, most recently from 8a1acfd to 8949b37 Compare March 2, 2023 22:20
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,
Copy link
Collaborator Author

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.

@gitKrystan gitKrystan requested a review from wycats March 2, 2023 22:35
@gitKrystan gitKrystan added bug Something isn't working enhancement New feature or request labels Mar 2, 2023
@gitKrystan gitKrystan changed the title Clean up user options schemas Handle 'true'/'false' stringified boolean options and better config error handling Mar 2, 2023
Copy link
Contributor

@wycats wycats left a 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';
Copy link
Contributor

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!

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.'
Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Input:

const Foo = EmberObject.extend({
@userAdded
whatever: undefined
});

Config:

Output:

@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.

Copy link
Contributor

@wycats wycats Mar 7, 2023

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';
Copy link
Contributor

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?

Copy link
Collaborator Author

@gitKrystan gitKrystan Mar 7, 2023

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>(
Copy link
Contributor

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.

Copy link
Contributor

@wycats wycats Mar 7, 2023

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?

Copy link
Collaborator Author

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.

@gitKrystan gitKrystan merged commit 38d9da2 into ember-codemods:master Mar 7, 2023
@gitKrystan gitKrystan deleted the better-args branch March 7, 2023 19:04
@gitKrystan gitKrystan removed the enhancement New feature or request label Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants