Skip to content

Conversation

erossignon
Copy link
Contributor

No description provided.

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

I think we could add eslint rules to do the same instead of creating our own check.

            rules: {
                "@eslint-react/naming-convention/filename": [
                    "warn",
                    { rule: "kebab-case" }
                ],
            },

@relu91
Copy link
Member

relu91 commented Sep 23, 2025

definitely better to use eslint to enforce it. I noticed that we are still using tslint. For that tool there is a native rule:
https://palantir.github.io/tslint/rules/file-name-casing/

In the future, we should migrate from tslint to typescript-eslint since it was deprecated. It should be easy, there is a convertion tool.

My proposal workplan to fix #1423

@erossignon erossignon force-pushed the feat/check_file_naming_convention branch from 12219a7 to f5d1e66 Compare September 24, 2025 06:50
@erossignon erossignon force-pushed the feat/check_file_naming_convention branch 2 times, most recently from 5b5ff9a to 828e9b7 Compare September 24, 2025 07:26
@erossignon erossignon changed the title chore(utils) add ts file naming convention checker #1423 fixing source filename to follow kebab-case convention #1423 Sep 24, 2025
@erossignon
Copy link
Contributor Author

erossignon commented Sep 24, 2025

  • First commit migrates to eslint 9.

The main difference is the introduction of eslint.config.mjs instead of eslintrc.json.

  • A few eslint packages have been outdated like eslint-node replaced with eslint-n.

  • standard typescript-eslint recommanded issues new errors that I have turned to warnings for the time being, We can decide to enforce them back one at a time, and fix the code at a later date.

  • n: also exhibit new issues, that are for the moment turned into warnings.

  • the great thing about using eslint.config.mjs over eslintrc.json, is that we can add comments to justify our eslint setting tweaks.

  • I have added the kebab-nameing rules, it works well and detect issues in cli, core, and binding-opcua.

  • the following commits fixes kebab naming in cli, core, and binding-opcua

  • finally, the latest commit enfore the kebab-nameing rule to be an error

  • I have removed the eslintrc.json files in workspace packages , They are now useless as when run in a package folder, eslint refers back to the top level config file.

@relu91
Copy link
Member

relu91 commented Sep 24, 2025

Great! Thank you for the hard work. This was very well needed. I have just a question, why use react naming convention when is already provided by typescript-eslint?

@danielpeintner
Copy link
Member

Many eslint warnings exist but are not shown in the overview.

If we agree to merge this PR we should open an issue to keep track of the warnings so that we can work on them..

@erossignon erossignon force-pushed the feat/check_file_naming_convention branch from d15814e to ea5d64d Compare September 24, 2025 08:59
Copy link
Member

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

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

Thank you for the changes and I am fine with them

@erossignon
Copy link
Contributor Author

erossignon commented Sep 25, 2025

. I have just a question, why use react naming convention when is already provided by typescript-eslint?

the react-naming-convention plug-in was suggested to us by @danielpeintner.

I have been scrutinizing https://typescript-eslint.io/rules/ and cannot find an equivalent for filename enforcement.

@relu91
Copy link
Member

relu91 commented Sep 25, 2025

What's wrong with this? https://typescript-eslint.io/rules/naming-convention/

@erossignon
Copy link
Contributor Author

You may have noticed the build is failing on a flaky tests.
I found the issue coming from a non-awaited async call.

Therefore I have activated the "@typescript-eslint/no-floating-promises": "warn", as rules.

This rule allow me to easily find the issue.

(we have 100 places where un-awaited promised functions call are missing catches, I only fixed in binding-opcua ) for the time being.

@erossignon
Copy link
Contributor Author

erossignon commented Sep 25, 2025

What's wrong with this? https://typescript-eslint.io/rules/naming-convention/

The rule naming-convention is about naming things inside the code ( like variables, classes, members). We are looking at naming source filename.

@erossignon erossignon changed the title fixing source filename to follow kebab-case convention #1423 fixing source filename to follow kebab-case convention and other eslint issues #1423 Sep 25, 2025
@relu91
Copy link
Member

relu91 commented Sep 25, 2025

What's wrong with this? https://typescript-eslint.io/rules/naming-convention/

The rule naming-convention is about naming things inside the code ( like variables, classes, members). We are looking at naming source filename.

Ah right! Then I am more confident to add a vendor-neutral ESLint plugin like this: https://github.com/DukeLuo/eslint-plugin-check-file, if for @danielpeintner is ok.

@erossignon
Copy link
Contributor Author

erossignon commented Sep 25, 2025

I think we should use the logger not the console

Good point, let's activate the "no-console-rule": "error" too ...

@erossignon erossignon force-pushed the feat/check_file_naming_convention branch from 4cff1d8 to d02f68d Compare September 25, 2025 09:20
@erossignon
Copy link
Contributor Author

erossignon commented Sep 25, 2025

Ah right! Then I am more confident to add a vendor-neutral ESLint plugin like this: https://github.com/DukeLuo/eslint-plugin-check-file, if for @danielpeintner is ok.

I now switched to eslint-plugin-check-file in ad4244c and also added folder names are in kebab-case too

Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

I think it is good in general. I noted some things I don't understand..

@erossignon erossignon force-pushed the feat/check_file_naming_convention branch from ad4244c to 2c07bbf Compare September 25, 2025 14:17
Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

I would go ahead, as this PR is getting too big. However, I have a question: What is the newly introduced licence.template.txt?

Finally, I would turn off the no-console rule for the examples globally instead of manually adding it to each file (To be addressed possibly in the future).

@erossignon
Copy link
Contributor Author

What is the newly introduced licence.template.txt?

@relu91 , check the comment from @danielpeintner above. He noticed that the file was missing, this file will help autofix the missing copyright notice.

@danielpeintner
Copy link
Member

I would go ahead, as this PR is getting too big.

I agree and we got 3 approvals also 👍

@danielpeintner danielpeintner merged commit 736f280 into eclipse-thingweb:master Sep 26, 2025
14 checks passed
@erossignon erossignon deleted the feat/check_file_naming_convention branch September 26, 2025 08:23
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.

4 participants