JS-599 Use a single method to find files #5165
Conversation
hashicorp-vault-sonar-prod
bot
changed the title
vdiez
force-pushed
the
single-find-files
branch
5 times, most recently
from
84d31a5 to
724ef5b
Compare
|
|
||
| export async function findFiles( | ||
| dir: string, | ||
| onFile: (file: Dirent) => Promise<void>, |
There was a problem hiding this comment.
why onFile and not return the files? If this using onFile, would a yield be good as well?
There was a problem hiding this comment.
it's just like an event watcher. I'm allowing the consumer of this function to pass any function with whatever they want to do with the file that was just found. i don't think a yield is applicable here, or I'm missing the point
| if (testPaths?.some(testPath => parent.startsWith(testPath))) { | ||
| fileType = 'TEST'; | ||
| } |
There was a problem hiding this comment.
should there be a isTestFile method with a good test suite? Or will it only be based on the provided paths?
There was a problem hiding this comment.
I'm still not sure about this. We only enter here if we call analyze-project without sending the files, allowing node.js to search for them instead. In that case only the path matters, but as I say I may clean up this after the POC I want to do next week
| for (const tsConfigPath of tsConfigPaths) { | ||
| const tsConfig = join(baseDir, tsConfigPath.trim()); | ||
| try { | ||
| await access(tsConfig); |
There was a problem hiding this comment.
yeah, probably should change that name. access it a function that checks if the file passes all filters: size, minified or not, and bundled or not
| const maxFiles = | ||
| typeof maxFilesForTypeChecking === 'undefined' | ||
| ? DEFAULT_MAX_FILES_FOR_TYPE_CHECKING | ||
| : maxFilesForTypeChecking; |
There was a problem hiding this comment.
null coalescing?
| const maxFiles = | |
| typeof maxFilesForTypeChecking === 'undefined' | |
| ? DEFAULT_MAX_FILES_FOR_TYPE_CHECKING | |
| : maxFilesForTypeChecking; | |
| const maxFiles = maxFilesForTypeChecking ?? DEFAULT_MAX_FILES_FOR_TYPE_CHECKING; |
There was a problem hiding this comment.
good point, but I changed that in latest commit, I want the default to be managed by the project-analyzer function, not here. now it's mandatory
| */ | ||
| export async function writeTSConfigFile(tsConfig: TsConfigJson): Promise<{ filename: string }> { | ||
| const filename = await new Promise<string>((resolve, reject) => { | ||
| tmp.file({ template: 'tsconfig-XXXXXX.json' }, function _tempFileCreated(err, path) { |
There was a problem hiding this comment.
will it be XXXXXX or it will be replaces?
There was a problem hiding this comment.
taken from tmp options:
https://www.npmjs.com/package/tmp#options
it was just a random filename, not even .json. I think it's better to have a more meaningful filename.
| expect(getTSConfigsCount()).toEqual(3); | ||
| }); | ||
|
|
||
| it('should write tsconfig file', async () => { |
There was a problem hiding this comment.
here you don't clearTSConfigs?
There was a problem hiding this comment.
we don't depend at all on what tsconfigs are known, this is 100% stateless
| const jsTsFiles = {}, | ||
| htmlFiles = {}, | ||
| yamlFiles = {}; | ||
| await findFiles( | ||
| projectPath, | ||
| async file => { | ||
| const filePath = toUnixPath(join(file.parentPath, file.name)); | ||
| const fileType = testPath && dirname(filePath).startsWith(testPath) ? 'TEST' : 'MAIN'; | ||
| if (isHtmlFile(filePath)) { | ||
| htmlFiles[filePath] = { fileType, filePath }; | ||
| } else if (isYamlFile(filePath)) { | ||
| yamlFiles[filePath] = { fileType, filePath }; | ||
| } else if (isJsTsFile(filePath)) { | ||
| const fileContent = await fs.readFile(filePath, 'utf8'); | ||
| if (accept(filePath, fileContent)) { | ||
| jsTsFiles[filePath] = { fileType, filePath, fileContent }; | ||
| } | ||
| } | ||
| }, | ||
| DEFAULT_EXCLUSIONS.concat((exclusions || '').split(',')), |
There was a problem hiding this comment.
you don't like helper methods?
There was a problem hiding this comment.
I want to remove that very soon...
| expect(await getAsyncIteratorValue(tsconfigs)).toEqual(undefined); | ||
| }); | ||
|
|
||
| it('when no tsconfigs, in SonarQube should generate tsconfig with all files', async () => { |
There was a problem hiding this comment.
So will you rename to: AnalysisSonarQube and AnalysisSonarLint? I would like this to at least be consistent. The names we have currently are: AnalysisWithProgram and AnalysisWithoutProgram, so please use a common strategy
There was a problem hiding this comment.
we can discuss this. not so long ago SQ was also using watch programs if there were Vue files present.
| it('when no tsconfigs, in SonarLint should generate tsconfig with wildcard', async () => { | ||
| clearTSConfigs(); | ||
| const baseDir = toUnixPath(join(fixtures, 'module')); | ||
| const files = await loadFiles(baseDir, {}); | ||
| const tsconfigs = getTSConfigsIterator(Object.keys(files), baseDir, true, 200); | ||
| const tsconfig = await getAsyncIteratorValue(tsconfigs); | ||
| expect(basename(tsconfig)).toMatch(/tsconfig-\w{6}\.json/); | ||
| expect(JSON.parse(await readFile(tsconfig, 'utf8'))).toMatchObject({ | ||
| compilerOptions: { | ||
| allowJs: true, | ||
| noImplicitAny: true, | ||
| }, | ||
| include: [`${baseDir}/**/*`], | ||
| }); | ||
| expect(await getAsyncIteratorValue(tsconfigs)).toEqual(undefined); | ||
| }); | ||
|
|
||
| it('when no tsconfigs, in SonarQube should generate tsconfig with all files', async () => { | ||
| clearTSConfigs(); | ||
| const baseDir = toUnixPath(join(fixtures, 'module')); | ||
| const files = await loadFiles(baseDir, {}); | ||
| const tsconfigs = getTSConfigsIterator(Object.keys(files), baseDir, false, 200); | ||
| const tsconfig = await getAsyncIteratorValue(tsconfigs); | ||
| expect(basename(tsconfig)).toMatch(/tsconfig-\w{6}\.json/); | ||
| expect(JSON.parse(await readFile(tsconfig, 'utf8'))).toMatchObject({ | ||
| compilerOptions: { | ||
| allowJs: true, | ||
| noImplicitAny: true, | ||
| }, | ||
| files: [`${baseDir}/file.ts`, `${baseDir}/string42.ts`], | ||
| }); | ||
| expect(await getAsyncIteratorValue(tsconfigs)).toEqual(undefined); | ||
| }); |
There was a problem hiding this comment.
same test with just a flag difference? Could you refactor?
There was a problem hiding this comment.
do you prefer it.each?
| export async function* getTSConfigsIterator( | ||
| files: string[], | ||
| baseDir: string, | ||
| sonarLint: boolean, |
There was a problem hiding this comment.
I'm not sure I like this boolean argument here
|
JS-599
logic around dealing with tsconfig files and lookup moves into projectAnalyzer.ts instead of program.ts, as it's there where it is used. find-files.ts helper has been removed as it was too convoluted
Once we are able to blacklist rules per extension (like
no-varfor HTML), the ruling will not need to search for files at all and just hand everything over to projectAnalyzer. Right now we still need to do it because ruling will remove that rule from the linter before proceeding with HTML analysisAdded a configuration object in the project analysis to reflect what sonar properties we may need to be sent from the Java part. Not all of them are supported at the moment.