Skip to content

Conversation

@Marvinrose
Copy link

Changes Made:

  • Upgraded ESLint to v9.21.0.
  • Migrated .eslintrc.yml to eslint.config.js using ESLint's migration tool.
  • Introduced eslint-plugin-jsdoc to replace deprecated built-in JSDoc support.
  • Implemented a conditional check for linebreak-style to support both Windows and Unix environments.
  • Removed .eslintignore (deprecated) and migrated ignored paths to eslint.config.js.
  • Verified the changes by running npx eslint . --fix ensuring no errors or warnings.

Testing:

  • Ran npx eslint . --fix successfully.
  • Verified ESLint rules applied correctly.
  • No breaking changes introduced.

My Environment:

  • Version used - ESLint Version: v9.21.0
  • Environment name and version (e.g. Chrome 39, node.js 5.4) - NPM Version: v10.8.2, Node.js Version: v18.20.7
  • Operating System and version (desktop or mobile) - Operating System: Windows 11 Home Single Language (Build 22631)
  • Link to your project - Accord Project Agreement Protocol (APAP) : https://github.com/accordproject/apap

@Marvinrose
Copy link
Author

Marvinrose commented Mar 6, 2025

Hi @sanketshevkar

I’ve submitted a PR that successfully resolves this issue.
The ESLint upgrade and migration are complete. All tests pass locally but the PR is currently blocked because the CI workflow requires approval before running.
Could you please approve the workflow so the tests can proceed? Let me know if anything needs to be changed.
Thanks!

Copy link
Member

@sanketshevkar sanketshevkar left a comment

Choose a reason for hiding this comment

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

Thanks for taking out time and making this contribution. This seems like a good start.

There are quite a few things that seems off and extra from what is required. I've flagged those. Can you please look into those?

If you're using any kind of AI tool, I'd like to suggest to please cross check its generated code and verify if the changes are needed are not. AI tools do tend to hallucinate and might generate extra code that makes maintainers' job to review the code difficult and needs extra effort.

@@ -0,0 +1,16 @@
import globals from 'globals';
Copy link
Member

Choose a reason for hiding this comment

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

Why does this require two config files? One with JS and other with mjs?

* limitations under the License.
*/

'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to remove these.

* @param {*} priorModels - known models
* @param {string} namespace - the namespace
* @return {*} the model
* @returns {*} the model
Copy link
Member

Choose a reason for hiding this comment

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

Why is this required?

"eslint": "9.21.0",
"eslint-plugin-jsdoc": "50.6.3",
"eslint-plugin-react": "7.37.4",
"globals": "16.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Why are globals and eslint-plugin-react packages required?

"semver": "7.6.3",
"typescript": "5.7.2"
"typescript": "5.7.2",
"typescript-eslint": "8.26.0"
Copy link
Member

Choose a reason for hiding this comment

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

Why is typescript-eslint required?

* @param {Array<{ source: string, destination: string }>} files - List of files to copy.
* @param {string} destDir - The destination directory.
* @returns {Promise<void>} - A promise that resolves when the files are copied.
*/
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to document this using jsdoc. We don't expose this method as APIs.

@sanketshevkar
Copy link
Member

Can you please comment on the issue so that we can assign you?

@Marvinrose
Copy link
Author

Marvinrose commented Mar 15, 2025

Hi @sanketshevkar

Thank you so much for the review and feedback.
Please assign it to me.

@sanketshevkar
Copy link
Member

@Marvinrose you'll have to comment on the issue for me to assign you.

@Marvinrose
Copy link
Author

@sanketshevkar I just did please

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.

2 participants