Conversation
xDivisionByZerox
added
c: infra
There was a problem hiding this comment.
I think you could remove the overrides block, because you have no rules in the top group anyway.
I would also recommend adding the vitest-eslint config from the main repo:
| "format": "prettier src --write", | ||
| "lint": "eslint src", | ||
| "lint:fix": "eslint src --fix" | ||
| "lint:fix": "eslint src --fix", |
There was a problem hiding this comment.
IMO these should also apply to the test folder/all files.
| "prettier": "^3.1.1", | ||
| "typescript": "~5.3.3" | ||
| "typescript": "~5.3.3", | ||
| "vitest": "^0.34.3" |
There was a problem hiding this comment.
Maybe also add eslint-plugin-vitest.
There was a problem hiding this comment.
Would that not be a searate concern that "setup tests" is?
There was a problem hiding this comment.
I would do it, but I don't consider it required.
If you add something new, you can also change directly related stuff like also edited the eslint config files.
IMO there is no big difference in adding the lint library for the tests and configuring the linter to run on tests.
Maybe do a full lint setup in a separate PR (Potentially just copy pasting the config from the main repo). 🤷
| const IGNORE_MODULES: LiteralUnion<keyof Faker>[] = [ | ||
| '_randomizer', | ||
| 'definitions', | ||
| 'rawDefinitions', | ||
| ]; |
There was a problem hiding this comment.
All other constants are defined in file scope instead of method local scope.
IMO we should keep it consistent.
There was a problem hiding this comment.
I was thinking about that, but the constants which I left in function scopes are only used there. You think consistency is more important than encapsulation?
There was a problem hiding this comment.
This is just my personal preference. IMO being file scoped is encapsulated enough. I also consider it helpful to list all data sets in one place, so that is easier to see what filters/data exist. This is part of my normal attempts to separate data from function, if they are too big to inline.
| const moduleRef = REMOVED_METHODS[moduleKey] ?? []; | ||
| return moduleRef.includes(entryKey); |
There was a problem hiding this comment.
In line 82 you check the value directly, I think you could do that here as well.
| const moduleRef = REMOVED_METHODS[moduleKey] ?? []; | |
| return moduleRef.includes(entryKey); | |
| return REMOVED_METHODS[moduleKey]?.includes(entryKey); |
There was a problem hiding this comment.
Note that your code will not result in the same value if moduleKey does not exist on REMOVED_METHODS. It will return undefined instead of false.
| const REMOVED_METHODS: { | ||
| [module: string]: ReadonlyArray<string>; | ||
| } = { | ||
| random: ['locale'], | ||
| }; |
2 participants
Description
Setup tests, using vitest, for the project.