[types/factory_girl] Refine initializer, attributes and buildOptions #50520

ishikawa · 2021-01-11T02:26:19Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change.
Follow the advice from the readme.
Avoid common mistakes.
Run npm test <package to test>.

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: <>
- Documentation https://github.com/simonexmachina/factory-girl

ishikawa · 2021-01-11T02:27:24Z

types/factory-girl/factory-girl-tests.ts

@@ -1,6 +1,6 @@
-import * as factory from "factory-girl";
+import * as factory from 'factory-girl';


I applied Prettier in e97e0bc.

ishikawa · 2021-01-11T02:27:50Z

types/factory-girl/index.d.ts

@@ -12,11 +12,20 @@ declare const factory: factory.Static;
 declare namespace factory {
    type Generator<T> = () => T;

-    type Definition<T> = T | Generator<T>;
+    type Definition<T> = T | Generator<T> | Promise<T>;


Each attribute can be Promise<T>.

https://github.com/simonexmachina/factory-girl/blob/f7ec9d5004021ff1334ab9440e9b5cc4fd1a18b0/src/Factory.js#L43

https://github.com/simonexmachina/factory-girl/blob/f7ec9d5004021ff1334ab9440e9b5cc4fd1a18b0/src/utils/asyncPopulate.js#L23

ishikawa · 2021-01-11T02:28:46Z

types/factory-girl/index.d.ts

-    }>;
+    };
+
+    type MaybeReadonlyArray<T> = T | ReadonlyArray<T>;


Most of Factory#...Many() function can take T or ReadonlyArray<T> for its attributes and options. So I defined a utility type for it.

https://github.com/simonexmachina/factory-girl/blob/f7ec9d5004021ff1334ab9440e9b5cc4fd1a18b0/src/Factory.js#L69

ishikawa · 2021-01-11T02:29:05Z

types/factory-girl/index.d.ts


        /**
         * Builds an array of model instances that are persisted
         */
-        buildMany<T>(name: string, num: number, attrs?: Attributes<Partial<T>>): Promise<T[]>;
-        buildMany<T>(name: string, attrs?: ReadonlyArray<Attributes<Partial<T>>>): Promise<T[]>;


As I investigated the implementation, Factory#buildMany(...) needs num parameter.

https://github.com/simonexmachina/factory-girl/blob/f7ec9d5004021ff1334ab9440e9b5cc4fd1a18b0/src/Factory.js#L99

ishikawa · 2021-01-11T02:29:30Z

types/factory-girl/index.d.ts

+            name: string,
+            num: number,
+            attrs?: MaybeReadonlyArray<Attributes<Partial<T>>>,
+            buildOptions?: MaybeReadonlyArray<BuildOptions>,


A Options<T> option can't be specified here. You can specify these options in Factory#define() or Factory#extend().

https://github.com/simonexmachina/factory-girl/blob/f7ec9d5004021ff1334ab9440e9b5cc4fd1a18b0/src/Factory.js#L110

danger-public · 2021-01-11T02:30:17Z

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

factory-girl (`unpkg`)

was missing the following properties:

ObjectAdapter
BookshelfAdapter
DefaultAdapter
MongooseAdapter
SequelizeAdapter

as well as these 1 other properties...

ReduxORMAdapter

Generated by 🚫 dangerJS against 30aa456

typescript-bot · 2021-01-11T02:34:58Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#50520	diff
Batch compilation
Memory usage (MiB)	38.5	39.8	+3.3%
Type count	2548	2588	+2%
Assignability cache size	157	194	+24%

Language service
Samples taken	252	328	+30%
Identifiers in tests	252	328	+30%
`getCompletionsAtPosition`
Mean duration (ms)	82.0	77.5	-5.6%
Mean CV	20.2%	17.4%
Worst duration (ms)	118.9	104.9	-11.7%
Worst identifier	map	scoreSequence
`getQuickInfoAtPosition`
Mean duration (ms)	82.2	77.5	-5.8%
Mean CV	19.3%	15.6%
Worst duration (ms)	112.5	113.6	+1.0%
Worst identifier	factory	options

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

typescript-bot · 2021-01-11T07:15:42Z

@ishikawa Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

factory-girl on npm, on unpkg
- owner-approval: @OlivierKamers

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by type definition owners, DT maintainers or others

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 50520,
  "author": "ishikawa",
  "headCommitOid": "30aa4563d38b96d01ea7955bb033fcf50dc17574",
  "lastPushDate": "2021-01-11T02:06:07.000Z",
  "lastActivityDate": "2021-01-28T14:38:12.000Z",
  "maintainerBlessed": false,
  "mergeOfferDate": "2021-01-28T13:06:48.000Z",
  "mergeRequestDate": "2021-01-28T14:38:12.000Z",
  "mergeRequestUser": "elcuy",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "popularityLevel": "Well-liked by everyone",
  "pkgInfo": [
    {
      "name": "factory-girl",
      "kind": "edit",
      "files": [
        {
          "path": "types/factory-girl/factory-girl-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/factory-girl/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "stackbuilders",
        "sestrella",
        "elcuy",
        "OlivierKamers"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "OlivierKamers",
      "date": "2021-01-28T13:06:10.000Z",
      "isMaintainer": false
    }
  ],
  "ciResult": "pass"
}

typescript-bot · 2021-01-11T07:15:42Z

🔔 @stackbuilders @sestrella @elcuy @OlivierKamers — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

typescript-bot · 2021-01-22T02:36:42Z

Re-ping @stackbuilders, @sestrella, @elcuy, @OlivierKamers:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

OlivierKamers

Looks good! I linked these definitions to our project and it fixes some @ts-expect-errors we had 🎉

OlivierKamers · 2021-01-28T12:54:22Z

types/factory-girl/index.d.ts

+            name: string,
+            num: number,
+            attrs?: MaybeReadonlyArray<Attributes<Partial<T>>>,
+            buildOptions?: BuildOptions | ReadonlyArray<BuildOptions>,


Minor comment: you can use your MaybeReadonlyArray utility type here

Thanks! I hadn't noticed that. But sorry, it has been already merged 🙇

typescript-bot · 2021-01-28T13:06:47Z

@ishikawa Everything looks good here. Great job! I am ready to merge this PR (at 30aa456) on your behalf.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@stackbuilders, @sestrella, @elcuy, @OlivierKamers: you can do this too.)

typescript-bot · 2021-01-28T13:06:48Z

@ishikawa Everything looks good here. Great job! I am ready to merge this PR (at 30aa456) on your behalf.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@stackbuilders, @sestrella, @elcuy, @OlivierKamers: you can do this too.)

elcuy · 2021-01-28T14:38:12Z

Ready to merge

typescript-bot · 2021-01-28T15:11:25Z

I just published @types/factory-girl@5.0.4 to npm.

@ishikawa

…zer, attributes and buildOptions by @ishikawa * prettier * update tests * update definitions

ishikawa added 3 commits January 11, 2021 11:02

prettier

e97e0bc

update tests

aac401a

update definitions

30aa456

ishikawa changed the title ~~Work/factory girl initializer and build options~~ [types/factory_girl] Refine initializer, attributes and buildOptions Jan 11, 2021

ishikawa commented Jan 11, 2021

View reviewed changes

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Jan 11, 2021

typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Jan 11, 2021

typescript-bot added the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label Jan 22, 2021

OlivierKamers approved these changes Jan 28, 2021

View reviewed changes

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Self Merge This PR can now be self-merged by the PR author or an owner and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Jan 28, 2021

typescript-bot moved this from Waiting for Code Reviews to Waiting for Author to Merge in New Pull Request Status Board Jan 28, 2021

typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Jan 28, 2021

typescript-bot merged commit 18bbb65 into DefinitelyTyped:master Jan 28, 2021

typescript-bot removed this from Recently Merged in New Pull Request Status Board Jan 28, 2021

ishikawa deleted the work/factory-girl-initializer-and-build-options branch January 28, 2021 22:30

kaznovac pushed a commit to kaznovac/DefinitelyTyped that referenced this pull request Mar 2, 2021

🤖 Merge PR DefinitelyTyped#50520 [types/factory_girl] Refine initiali…

1ad048f

…zer, attributes and buildOptions by @ishikawa * prettier * update tests * update definitions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[types/factory_girl] Refine initializer, attributes and buildOptions #50520

[types/factory_girl] Refine initializer, attributes and buildOptions #50520

ishikawa commented Jan 11, 2021 •

edited

ishikawa Jan 11, 2021

ishikawa Jan 11, 2021

ishikawa Jan 11, 2021

ishikawa Jan 11, 2021 •

edited

ishikawa Jan 11, 2021

danger-public commented Jan 11, 2021

typescript-bot commented Jan 11, 2021

typescript-bot commented Jan 11, 2021 •

edited

typescript-bot commented Jan 11, 2021

typescript-bot commented Jan 22, 2021

OlivierKamers left a comment

OlivierKamers Jan 28, 2021

ishikawa Jan 28, 2021

typescript-bot commented Jan 28, 2021

typescript-bot commented Jan 28, 2021

elcuy commented Jan 28, 2021

typescript-bot commented Jan 28, 2021

		@@ -1,6 +1,6 @@
		import * as factory from "factory-girl";
		import * as factory from 'factory-girl';

[types/factory_girl] Refine initializer, attributes and buildOptions #50520

[types/factory_girl] Refine initializer, attributes and buildOptions #50520

Conversation

ishikawa commented Jan 11, 2021 • edited

ishikawa Jan 11, 2021

Choose a reason for hiding this comment

ishikawa Jan 11, 2021

Choose a reason for hiding this comment

ishikawa Jan 11, 2021

Choose a reason for hiding this comment

ishikawa Jan 11, 2021 • edited

Choose a reason for hiding this comment

ishikawa Jan 11, 2021

Choose a reason for hiding this comment

danger-public commented Jan 11, 2021

factory-girl (unpkg)

typescript-bot commented Jan 11, 2021

typescript-bot commented Jan 11, 2021 • edited

1 package in this PR

Code Reviews

Status

typescript-bot commented Jan 11, 2021

typescript-bot commented Jan 22, 2021

OlivierKamers left a comment

Choose a reason for hiding this comment

OlivierKamers Jan 28, 2021

Choose a reason for hiding this comment

ishikawa Jan 28, 2021

Choose a reason for hiding this comment

typescript-bot commented Jan 28, 2021

typescript-bot commented Jan 28, 2021

elcuy commented Jan 28, 2021

typescript-bot commented Jan 28, 2021

ishikawa commented Jan 11, 2021 •

edited

ishikawa Jan 11, 2021 •

edited

factory-girl (`unpkg`)

typescript-bot commented Jan 11, 2021 •

edited