Skip to content

Refactor code to Eslint 9.x#3375

Merged
georgedias merged 17 commits intomainfrom
updateEslintConfig
Mar 19, 2025
Merged

Refactor code to Eslint 9.x#3375
georgedias merged 17 commits intomainfrom
updateEslintConfig

Conversation

@georgedias
Copy link
Contributor

@georgedias georgedias commented Mar 16, 2025

  • Refactor code to comply with eslint 9.x
  • Remove unnecessary any
  • Replace any with `unknown when possible

Summary of using any or unknown

Feature any unknown
Type Safety ❌ No ✅ Yes
Requires Type Checking ❌ No ✅ Yes
Can Be Assigned Any Type ✅ Yes ✅ Yes
Can Be Used Without Type Checking ✅ Yes ❌ No
  • Use unknown for better type safety and explicit checks
  • Only use any if you truly need unchecked, flexible behavior

Summary of linting configuration

ESLint packages used:

  • eslint –> The core ESLint package. It is the base linter.
    NOTE: Styles have moved from eslint -> @stylistic/eslint-plugin (js and ts)
    Built-in stylistic rules for JavaScript and Typescript
    We are still using legacy config, so we need to install v3.x with
    npm i -D @stylistic/eslint-plugin@3

  • @stylistic/eslint-plugin -> Used for general stylistic rules, which can be
    applied to both JavaScript and TypeScript.
    See here for additional information on ESLint Stylistic Stylistic Formatting
    for ESLint https://eslint.style/

  • eslint-plugin-n -> This package provides additional ESLint rules for Node.js
    Originally the these were provided by the eslint-plugin-node that it is
    no longer maintained

  • @eslint/js –> This package provides core ESLint rules for JavaScript. It’s
    necessary to allow ESLint to enforce JavaScript best practices.

  • typescript-eslint –> Tooling which enables the use of TypeScript with ESLint.
    This package is the entrypoint to consume additional tooling with ESLint.
    The package exports the following:
    config -> A utility function for creating type-safe flat configs
    configs -> Shared ESLint (flat) configs
    parser -> A re-export of @typescript-eslint/parser
    plugin -> A re-export of @typescript-eslint/eslint-plugin
    Notes: We still need to install the parser and plugin separately

    • @typescript-eslint/parser –> This parser allows ESLint to understand TypeScript
      syntax. Without it, ESLint won’t be able to process TypeScript files correctly.
    • @typescript-eslint/eslint-plugin –> This ESLint plugin provides additional
      TypeScript-specific rules.

Optional package used that are very useful for best practice:

  • eslint-plugin-unicorn –> A plugin that provides a collection of additional
    rules that enforce best practices and modern JavaScript standards. It’s
    optional but useful for enforcing stricter code quality.

  • eslint-formatter-stylish –> A package that provides a stylish formatter for
    ESLint. It’s optional but useful for a more readable output.


May need to use the chai plugin in the future
npm install --save-dev eslint-plugin-chai-friendly
And configure it into plugins: { "chai-friendly": chaiPLugin }

Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
@georgedias georgedias changed the title Refractor code to Eslint 9.x Refactor code to Eslint 9.x Mar 17, 2025
georgedias and others added 6 commits March 18, 2025 15:01
Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
@georgedias georgedias self-assigned this Mar 18, 2025
@georgedias georgedias linked an issue Mar 18, 2025 that may be closed by this pull request
3 tasks
@georgedias georgedias requested review from DMedina6 and em-c-rod March 19, 2025 16:22
@em-c-rod
Copy link
Contributor

em-c-rod commented Mar 19, 2025

Do you think we should put the notes you have in this PR above in any documentation we have?

Sorry, I now see that you did this already.

README.md Outdated
* [Apply Attestations](#apply-attestations)


### [Get Help with Converts](#converts)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### [Get Help with Converts](#converts)
### [Get Help with Convert](#convert)

To me it makes more sense without the s. We could also be even more clear and say:

Get Help with Convert Commands

README.md Outdated
```
[top](#usage)
### Convert
### Converts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Converts
### Convert

Alternatively:

Convert Commands

README.md Outdated

Want to Recommend or Help Develop a Converter? See [on how to get started](https://github.com/mitre/saf/wiki/How-to-recommend-development-of-a-mapper) 📰

[top](#get-help-with-converts)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[top](#get-help-with-converts)
[top](#get-help-with-convert)

// for ESLint https://eslint.style/
//
// "eslint-plugin-n" -> This package provides additional ESLint rules for Node.js
// Originally the these were provided by the eslint-plugin-node that it is
Copy link
Contributor

@em-c-rod em-c-rod Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Originally the these were provided by the eslint-plugin-node that it is
// Originally these were provided by the eslint-plugin-node which is

}

case 'xlsx': {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you know when to disable the no-explicit-any vs. change the type to unknown?

Copy link
Contributor Author

@georgedias georgedias Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I was able to determine the type I made the changes, otherwise I left it at any and disabled the linting.
Basically I follow the rule, if I can test to null and knew what the side effects were I changed. Most of the any for emassser I know what types to use or knew enough to create an interface.

for (const key in controls) {

for (const key in controls) { // skipcq: JS-0051
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do you reference what each number lint rule means?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to find out, but couldn't. I use DeepSource to tell me about the finding

const hideNulls: boolean = conf.displayNulls !== 'true'
const showEpoch: boolean = (conf.displayDateTime === 'true')
const debugging: boolean = (conf.debugging === 'true')
const hideNulls = !conf.displayNulls
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still work because it seems like the previous check was checking against a string rather than boolean type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is correct, the configuration where these are set changed.

};
expect(omitHDFChangingFields(output)).to.eql(omitHDFChangingFields(expected))
assert.isEmpty(stderr)
//expect(stderr).to.be.empty
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//expect(stderr).to.be.empty

@@ -1,5 +1,5 @@
/* eslint-disable array-bracket-newline */
/* eslint-disable array-element-newline */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove these newlines

@@ -1,5 +1,5 @@
/* eslint-disable array-bracket-newline */
/* eslint-disable array-element-newline */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove these newlines?

@@ -1,5 +1,5 @@
/* eslint-disable array-bracket-newline */
/* eslint-disable array-element-newline */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra newlines

@@ -1,5 +1,5 @@
/* eslint-disable array-bracket-newline */
/* eslint-disable array-element-newline */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra newlines

@@ -1,5 +1,5 @@
/* eslint-disable array-bracket-newline */
/* eslint-disable array-element-newline */

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will hold from commenting in each situation. Whenever we update to remove the extra newlines, we can do a sweep to check for all of them.

expect(fs.lstatSync((`${tmpobj.name}/RHEL_7/my-report.md`)).isFile()).to.be.true // skipcq: JS-0354
const isReportFile = fs.lstatSync((`${tmpobj.name}/RHEL_7/my-report.md`)).isFile()
assert.isTrue(isReportFile)
// expect(fs.lstatSync((`${tmpobj.name}/RHEL_7/my-report.md`)).isFile()).to.be.true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// expect(fs.lstatSync((`${tmpobj.name}/RHEL_7/my-report.md`)).isFile()).to.be.true

expect(fs.lstatSync((`${tmpobj.name}/delta.md`)).isFile()).to.be.true // skipcq: JS-0354
const isFile = fs.lstatSync((`${tmpobj.name}/delta.md`)).isFile()
assert.isTrue(isFile)
// expect(fs.lstatSync((`${tmpobj.name}/delta.md`)).isFile()).to.be.true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// expect(fs.lstatSync((`${tmpobj.name}/delta.md`)).isFile()).to.be.true

expect(text).to.not.be.null // skipcq: JS-0354
} catch (error: any) {
expect(error.message).to.equal('Request failed with status code 404')
// expect(text).to.not.be.null // skipcq: JS-0354
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// expect(text).to.not.be.null // skipcq: JS-0354

expect(appDiv).to.not.be.null // skipcq: JS-0354
} catch (error: any) {
expect(error.message).to.equal('Request failed with status code 404')
// expect(appDiv).to.not.be.null // skipcq: JS-0354
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// expect(appDiv).to.not.be.null // skipcq: JS-0354

Copy link
Contributor

@em-c-rod em-c-rod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments and questions. Thank you for your work on updating all of this linting.

Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
georgedias and others added 4 commits March 19, 2025 15:49
Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
Signed-off-by: George M Dias <GDIAS@MITRE.ORG>
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
18.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@georgedias georgedias merged commit d28b149 into main Mar 19, 2025
16 of 17 checks passed
@mergify mergify bot deleted the updateEslintConfig branch March 19, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update inspec_profile

3 participants