Simplified initialSetup() function

[jamessimone]

• swapped let for const when appropriate
• simplified initialSetup() function

[jamessimone]

@mitchspano just wanted to get a little PR going here, but I have a much bigger question - any reason for not using TypeScript for this repo? It would make refactoring some of these functions a lot easier (especially places where the sync version of fs functions are being used at present) and allow you to express things like the severity levels being returned from the scanner in a type-safe way

[jamessimone]

As an example of why I find TS to be the ultimate helper for these things - it found the (obvious) bug in the solution I just submitted:

I'll snap off another commit with that fix

[mitchspano]

Hey @jamessimone thank you so much for your contribution!

Regarding TypeScript, I would be definitely open to refactoring this to TypeScript - I only chose JavaScript because of my familiarity with the language and the readily available documentation.

If we can make this work with TypeScript it would provide the benefits you mentioned above and (perhaps selfishly) be an excellent learning opportunity for me to get familiar with a new language.

[jamessimone]

@mitchspano I included another example of how helpful TypeScript can be in the PR I tagged you in on my own fork. There is going to be (in my experience locally developing this application) a significant pain point in keeping node_modules as part of this project's source code, but we would also need to prove out that the tools which bundle dependencies with a TypeScript action actually include the sfdx-cli dependency (since that is being invoked via a child process, which probably precludes the ability for tree shaking to find the dependency properly).

Regardless - I think there's a lot of potential to do additional cleanup even if the project remains entirely javascript-based. Here's an example of what I'm talking about (which I only have locally):

async function main() {
  const { inputs, pullRequest, scannerCliArgs } = initialSetup();
  validatePullRequestContext(pullRequest);
  const filePathToChangedLines = getDiffInPullRequest(pullRequest);
  await recursivelyMoveFilesToTempFolder(filePathToChangedLines);
  const diffFindings = performStaticCodeAnalysisOnFilesInDiff(scannerCliArgs);
  const existingComments = await getExistingComments(pullRequest);
  const filePathToComments = filterFindingsToDiffScope(
    diffFindings,
    filePathToChangedLines,
    inputs,
    pullRequest
  );
  writeComments(
    existingComments,
    filePathToComments,
    pullRequest,
    hasHaltingError // this is still a "global" variable, but now it's the only one
  );
}

Changes like these are meant to remove variables from global state, creating a more idiomatic program control flow. I'm happy to re-create these changes in the pure javascript version here as well.

[mitchspano]

Yes, I would like to remove node_modules, however when writing a Javascript GitHub action they actually instruct you to include node_modules:

There is an alternative which involves using vercel to compile all the contents of node_modules into one file, but when I tried following the provided instructions, I was unable to get it to work, so I fell-back on ye olden node_modules folder.

I have created this issue to track the work of removal of node_modules in favor of Vercel.

[mitchspano]

These changes look great - thank you for the simplification!

Were you able to test this running in an action's runtime from your fork?
The testing of this project has been quite challenging - it involves creating a tag and a release, then running the test-release action in a separate repo 😵‍💫.

If you weren't able to test in an action's runtime, I can create a separate "release" branch for these pending changes to be bundled into a release and tested in a separate repo before being merged into main.

[jamessimone]

@mitchspano I'm aware of the node_module "need" for actions, yes. It would definitely be preferable to get around that by bundling. Vercel is just one of many options - tsbuild is another; ironically I believe rollup also can do TS transpilation.

In the past I've used act to test locally, as others were telling you about. As this was a strict refactor, I haven't yet had the chance to do that.

[jamessimone]

I also think testing can be much simplified - it should be relatively simple to set up Jest tests for this repo to unit test all changes properly without even having to run the action locally. You certainly should not need to be running things in a separate repo.

[mitchspano]

I was able to test this by creating a separate branch, tag, and release which included these changes and execute those in a separate test repository.

[mitchspano]

These look great - thank you for the contribution @jamessimone!