Skip to content
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

Source code todo list v2 #1044

Closed
ST-DDT opened this issue Jun 9, 2022 · 7 comments
Closed

Source code todo list v2 #1044

ST-DDT opened this issue Jun 9, 2022 · 7 comments
Labels
c: chore PR that doesn't affect the runtime behavior help wanted Extra attention is needed p: 1-normal Nothing urgent

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Jun 9, 2022

This list contains all TODOs as of c5ac124.

Done Issue/PR File Reporter Description
#1066 src/modules/address/index.ts:426 @ST-DDT Allow coordinate parameter to be [string, string].
- src/modules/address/index.ts:455 @Shinigami92 Provide an option property to provide custom circumferences.
- src/modules/helpers/index.ts:225 unknown implement for letters e.g. [0-9a-zA-Z] etc.
- src/modules/helpers/index.ts:398 @Shinigami92 We want to remove this default value, but currently it's not possible because some definitions could be empty
- src/modules/helpers/index.ts:424 @Shinigami92 We want to remove this default value, but currently it's not possible because some definitions could be empty
- src/modules/image/providers/lorempicsum.ts:6 @ST-DDT Rename to picsum?
- src/modules/image/providers/lorempicsum.ts:69 @ST-DDT This method does the same as image url, maybe generate a seed, if it is missig?
#1065 src/modules/image/providers/lorempicsum.ts:80 @ST-DDT Deprecate this method as it is duplicate and has nothing to do with lorempicsum.
#1069 src/modules/image/providers/lorempixel.ts:47 @ST-DDT Deprecate this method as it is duplicate and has nothing to do with lorempixel.
#1046 src/modules/image/providers/unsplash.ts:7 @ST-DDT Remove unused(?) constant
#1062 src/modules/image/providers/unsplash.ts:37 @ST-DDT Deprecate this method as it is duplicate and has nothing to do with unsplash.
#1059 src/modules/internet/index.ts:269 @Shinigami92 We may want to return a IPv4 or IPv6 address here in a later major release
- src/modules/name/index.ts:24 @Shinigami92 Remove fallback empty object when strict: true
#1080 src/modules/name/index.ts:162 @Shinigami92 Add possibility to have a prefix together with a suffix
#1080 src/modules/name/index.ts:168 @Shinigami92 Not sure if this fallthrough is wanted
- src/modules/name/index.ts:227 @Shinigami92 Add female_suffix and male_suffix
#1063 src/modules/phone/index.ts:28 @pkuczynski simplify name to number()
#1089 src/modules/random/index.ts:236 @Shinigami92 Tests covered (count, options), but they were never typed like that
#1050 src/modules/system/index.ts:22 @ST-DDT Replace with Array.from(Set)
#395 src/modules/system/index.ts:176 @prisis add a parameter to have the possibility to have one or two ext on file.
- test/all_functional.spec.ts:23 @ST-DDT these are TODOs (usually broken locale files)
- test/all_functional.spec.ts:77 @ST-DDT Use random seed once there are no more failures
- test/all_functional.spec.ts:91 @ST-DDT Remove once there are no more failures
- test/all_functional.spec.ts:110 @ST-DDT Use random seed once there are no more failures
- test/finance.spec.ts:228 unknown add support for more currency and decimal options
- test/finance.spec.ts:403 unknown implement checks for each format with regexp
#1115 test/internet.spec.ts:159 @Shinigami92 When providing params to email, it produces some not that predictable data
- test/internet.spec.ts:480 @Shinigami92 Make tests more strict
- test/internet.spec.ts:494 @Shinigami92 Understand what's going on
- test/internet.spec.ts:586 @Shinigami92 I would say a non memorable password should satisfy validator.isStrongPassword, but it does not currently
- test/internet.spec.ts:600 @Shinigami92 This should definitely be a strong password, but it doesn't :(

Legend:

  • ❓ not yet fixed/fix not merged yet
  • ✔ fixed
  • ⛔ won't fix

Please leave a comment if you are going to tackle a particular TODO to avoid conflicts.

The table was generated using this script
grep -r . -n -e "TODO" |\
	grep -v "/.git/" |\
	grep -v "TODO: Insert Title for ${locale}" |\
	sed 's/^\.\///' |\
	sed -E 's/: *\/\/ ?TODO:? / /' |\
	sed -E 's/@?ST-DDT/@ST-DDT/' |\
	sed -E 's/(:[0-9]+) /\1 | /' |\
	sed -E 's/\| ([^@])/| unknown | \1/' |\
	sed -E 's/ [0-9-]+: / | /' |\
	sed -E 's/:([0-9]+) / | \1 /' |\
	sed -E "s/([^ ]+) \| ([0-9]+)/[\1:\2](https:\/\/github.com\/faker-js\/faker\/blob\/$(git rev-parse HEAD)\/\1#L\2)/" |\
	sed 's/^/| ❓ | - | /' |\
	sed 's/$/ |/'
@ST-DDT ST-DDT added help wanted Extra attention is needed c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent labels Jun 9, 2022
@ST-DDT ST-DDT added this to the v7 - Current Major milestone Jun 9, 2022
@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Jun 9, 2022
kris71990 added a commit to kris71990/faker that referenced this issue Jun 9, 2022
@xDivisionByZerox
Copy link
Member

I would propose to set the rename of LoremPicsum to Picsum to won't fix.
This line

src/modules/image/providers/lorempicsum.ts:6 @ST-DDT Rename to picsum?

The URL is just 'https://picsum.photo' but the website is named 'Lorem Picsum'. So the class name checks out IMO.

@ST-DDT
Copy link
Member Author

ST-DDT commented Jun 12, 2022

Ok, then please remove the todo and maybe adjust the jsdocs slightly.

@xDivisionByZerox
Copy link
Member

About the todo in the random module:

// TODO @Shinigami92 2022-02-14: Tests covered `(count, options)`, but they were never typed like that

I can't find any test case that tests that specific signature. Can this todo be removed from the code base and set as fixed in the list?

@ST-DDT
Copy link
Member Author

ST-DDT commented Jun 18, 2022

Sure

@xDivisionByZerox
Copy link
Member

@Shinigami92 can you explain your to-do comment in the internet test file?

// TODO @Shinigami92 2022-02-11: Understand what's going on

I don't see what's wrong here.

@Shinigami92
Copy link
Member

@xDivisionByZerox I assume I don't understand what

Math.floor((this.faker.datatype.number(256) + base) / 2)
.toString(16)
.padStart(2, '0');

does
I'm a bit confused by passing 3x 100 into internet.color still produces random values, this is a good thing and not wrong at all, I just don't understand 🤷
Would be happy if someone could explain and document that

@ST-DDT
Copy link
Member Author

ST-DDT commented Sep 8, 2022

@xDivisionByZerox Will create an issue for each TODO that is still in the code.

@ST-DDT ST-DDT closed this as completed Sep 8, 2022
Repository owner moved this from Todo to Done in Faker Roadmap Sep 8, 2022
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 help wanted Extra attention is needed p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants