Skip to content

fix: some return types#307

Merged
Shinigami92 merged 3 commits into
faker-js:mainfrom
ejcheng:fix_some_return_types
Jan 28, 2022
Merged

fix: some return types#307
Shinigami92 merged 3 commits into
faker-js:mainfrom
ejcheng:fix_some_return_types

Conversation

@ejcheng

@ejcheng ejcheng commented Jan 26, 2022

Copy link
Copy Markdown
Member

Partially fixes #305
@ejcheng ejcheng requested a review from a team as a code owner January 26, 2022 03:23
@ejcheng ejcheng requested a review from damienwebdev January 26, 2022 03:24
@ejcheng ejcheng changed the title fix: some implicit return types fix: some return types Jan 26, 2022
@ejcheng ejcheng force-pushed the fix_some_return_types branch from 73f3d19 to 0a5f5ad Compare January 26, 2022 04:26
@Shinigami92

Shinigami92 commented Jan 26, 2022

Copy link
Copy Markdown
Member

Currently one of our problems is that arrayElement doesn't guarantee type-safetynes (regrading to compile-time).
Our definitions currently are just mostly arrays, but could be anything 🤷
I know that there are currently mostly string[] and we could just take over the types of @types/faker, but I would like to double-check that instead of assuming.
@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label Jan 26, 2022
ST-DDT
ST-DDT previously approved these changes Jan 26, 2022
View reviewed changes
@ST-DDT

ST-DDT commented Jan 26, 2022

Copy link
Copy Markdown
Member

IMO we should set these to the expected return types. Double checking with the existing ones would still be a good idea.
doughlass
doughlass previously approved these changes Jan 26, 2022
View reviewed changes
@ejcheng

ejcheng commented Jan 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

IMO we should set these to the expected return types. Double checking with the existing ones would still be a good idea.

All of the ones in this PR were checked against @types/faker latest version (before corruption), they all seem reasonable and match up.
@ST-DDT ST-DDT requested a review from a team January 26, 2022 12:10
@ejcheng ejcheng dismissed stale reviews from doughlass and ST-DDT via 9577d68 January 26, 2022 18:18
@ST-DDT

ST-DDT commented Jan 26, 2022

Copy link
Copy Markdown
Member

A probably wrong commit ended up here.
ST-DDT
ST-DDT requested changes Jan 26, 2022
View reviewed changes
Comment thread .github/ISSUE_TEMPLATE/config.yml Outdated
@ejcheng ejcheng force-pushed the fix_some_return_types branch from 9577d68 to 0a5f5ad Compare January 26, 2022 19:03
@ejcheng ejcheng requested a review from ST-DDT January 26, 2022 19:04
@demipel8 demipel8 mentioned this pull request Jan 27, 2022
Merged
@demipel8

demipel8 commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

Wrong mention, sorry.
@Shinigami92 Shinigami92 merged commit 4ca61ca into faker-js:main Jan 28, 2022
bmenant pushed a commit to bmenant/faker that referenced this pull request Mar 11, 2022
@ejcheng @bmenant
fix: some return types (faker-js#307)
ae16f89
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
@ejcheng @demipel8
fix: some return types (faker-js#307)
b1b1438
demipel8 pushed a commit to demipel8/faker that referenced this pull request Mar 11, 2022
@ejcheng @demipel8
fix: some return types (faker-js#307)
963a27d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: chore PR that doesn't affect the runtime behavior

5 participants

@ejcheng @Shinigami92 @ST-DDT @demipel8 @doughlass