Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(#2628): generate eol LF config #2635

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+24 −72
Open

fix(#2628): generate eol LF config #2635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .yarnrc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ packageExtensions:
"@swc/types": "*"
"@typescript-eslint/rule-tester@*":
dependencies:
"@typescript-eslint/parser": ~8.17.0
"@typescript-eslint/parser": ~8.18.0
"@angular-eslint/eslint-plugin-template@*":
dependencies:
"@typescript-eslint/types": "^8.0.0"
Expand Down
2 changes: 1 addition & 1 deletion packages/@ama-sdk/schematics/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@
"@swc/helpers": "~0.5.0",
"@commitlint/cli": "^19.0.0",
"@commitlint/config-conventional": "^19.0.0",
"@typescript-eslint/eslint-plugin": "~8.17.0",
"@typescript-eslint/eslint-plugin": "~8.18.0",
"jest-junit": "~16.0.0",
"lint-staged": "^15.0.0",
"minimist": "^1.2.6",
Expand Down
6 changes: 3 additions & 3 deletions packages/@o3r/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,8 @@
"@o3r/store-sync": "workspace:^",
"@stylistic/eslint-plugin": "~2.7.0",
"@types/jest": "~29.5.2",
"@typescript-eslint/eslint-plugin": "~8.17.0",
"@typescript-eslint/parser": "~8.17.0",
"@typescript-eslint/eslint-plugin": "~8.18.0",
"@typescript-eslint/parser": "~8.18.0",
"angular-eslint": "~18.4.0",
"cpy-cli": "^5.0.0",
"eslint": "~9.14.0",
Expand All @@ -176,7 +176,7 @@
"jest-preset-angular": "~14.2.0",
"jsonc-eslint-parser": "~2.4.0",
"nx": "~19.8.0",
"typescript-eslint": "~8.17.0",
"typescript-eslint": "~8.18.0",
"zone.js": "~0.14.2"
},
"engines": {
Expand Down
16 changes: 16 additions & 0 deletions packages/@o3r/create/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
type SpawnSyncOptionsWithBufferEncoding,
} from 'node:child_process';
import {
existsSync,
readFileSync,
writeFileSync,
} from 'node:fs';
Expand Down Expand Up @@ -265,6 +266,21 @@ const prepareWorkspace = (relativeDirectory = '.', projectPackageManager = 'npm'
}
}

if (argv.minimal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure to understand this option.
If it is needed, you should also provide documentation regarding it in readme.
(and potentially tests :D)

Copy link
Contributor Author

@matthieu-crouzet matthieu-crouzet Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is already documented here, you can go on this link https://v17.angular.io/cli/new#options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In their case they are not generating the .editorconfig if they have the option minimal set to true

Copy link
Contributor

@kpanot kpanot Dec 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is already documented here, you can go on this link v17.angular.io/cli/new#options

This is not really a fare answer :D.
The option itself is not documented but, as part of Angular option, is supported.
I think you can add a comment (with a link potentially) in the code regarding it.

In their case they are not generating the .editorconfig if they have the option minimal set to true

Would be worse to add it as comment as well in the code

const editorconfigPath = resolve(cwd, '.editorconfig');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it implemented in this file instead of be part of core?
To remember, this file should contain exclusively what it mandatory to run Ng generation and the add of Otter core.
If it is not the case of these file updates you may need to move them in core.

I also think that in any case, as the generators require to be have the lf constrain, the core schematics should do this update (as well if it is mandatory here).

Copy link
Contributor Author

@matthieu-crouzet matthieu-crouzet Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For me, this file it's part of the monorepo configuration, I would expect to have it defined directly when I create the monorepo.
I don't expect it to change when someone run ng add @o3r/core on an exisiting monorepo.

Copy link
Contributor Author

@matthieu-crouzet matthieu-crouzet Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What do you think about that @AmadeusITGroup/otter_admins ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think this should only be generated at create time, not on ng add

Copy link
Contributor

@cpaulve-1A cpaulve-1A Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Same as Florian and Matthieu. I would not mess with an existing project.
It makes sense to change it when the editorconfig file is created, and in this case the file is created during the angular create script.

Copy link
Contributor

@kpanot kpanot Dec 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But this is not at all the purpose of the create script.
If it is a mandatory for our generators then as soon as we add Otter to a project we may need this change.
If it is not, but part of the Otter guidelines, then it should be bring by a ng add (even if it is done only once).

Please remember that the create script is only to initialize a ng repo and start an Otter setup, it should remain as simple as possible and it should not be the one applying guidelines (or setup default value) that would not be available to people who are not generating the repository from scratch with the create.

For me the correct behavior is the following one:
On Otter setup, if the .editorconfig file is present, we default the value of [*].end_of_line to lf (if no value already set). If the file does not exist, we create if root: true and [*].end_of_line: true.
(same logic for .gitattributes file).

const editorconfig = existsSync(editorconfigPath) ? readFileSync(editorconfigPath, { encoding: 'utf8' }) : '';
const newEditorconfig = /\[[*]\]/.test(editorconfig)
? editorconfig.replace(/(\[[*]\])/, '$1\nend_of_line = lf')
: editorconfig.concat('[*]\nend_of_line = lf');
writeFileSync(editorconfigPath, newEditorconfig);
}
const gitAttributesPath = resolve(cwd, '.gitattributes');
writeFileSync(
gitAttributesPath,
(existsSync(gitAttributesPath) ? readFileSync(gitAttributesPath, { encoding: 'utf8' }) : '')
.concat('* text eol=lf')
);

exitProcessIfErrorInSpawnSync(INSTALL_PROCESS_ERROR_CODE, spawnSync(runner, ['install'], spawnSyncOpts));
};

Expand Down
6 changes: 3 additions & 3 deletions packages/@o3r/eslint-config-otter/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@
"typescript": "^5.5.4"
},
"generatorDependencies": {
"@typescript-eslint/eslint-plugin": "~8.17.0",
"@typescript-eslint/parser": "~8.17.0",
"@typescript-eslint/utils": "~8.17.0",
"@typescript-eslint/eslint-plugin": "~8.18.0",
"@typescript-eslint/parser": "~8.18.0",
"@typescript-eslint/utils": "~8.18.0",
"eslint": "~9.14.0",
"eslint-plugin-jsdoc": "~50.2.0",
"eslint-plugin-unicorn": "^56.0.0"
Expand Down
64 changes: 0 additions & 64 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14892,24 +14892,6 @@ __metadata:
languageName: node
linkType: hard

"@typescript-eslint/parser@npm:~8.17.0":
version: 8.17.0
resolution: "@typescript-eslint/parser@npm:8.17.0"
dependencies:
"@typescript-eslint/scope-manager": "npm:8.17.0"
"@typescript-eslint/types": "npm:8.17.0"
"@typescript-eslint/typescript-estree": "npm:8.17.0"
"@typescript-eslint/visitor-keys": "npm:8.17.0"
debug: "npm:^4.3.4"
peerDependencies:
eslint: ^8.57.0 || ^9.0.0
peerDependenciesMeta:
typescript:
optional: true
checksum: 10/464981e1424e4a7849ca7253b54092a67d33130d28ecf492efd56d5ce69e640a876b7f84e59f1e368e763125432c34e7de28d78c0eef1bda4e9a9d52de0ccac5
languageName: node
linkType: hard

"@typescript-eslint/rule-tester@npm:~8.18.0":
version: 8.18.1
resolution: "@typescript-eslint/rule-tester@npm:8.18.1"
Expand All @@ -14926,16 +14908,6 @@ __metadata:
languageName: node
linkType: hard

"@typescript-eslint/scope-manager@npm:8.17.0":
version: 8.17.0
resolution: "@typescript-eslint/scope-manager@npm:8.17.0"
dependencies:
"@typescript-eslint/types": "npm:8.17.0"
"@typescript-eslint/visitor-keys": "npm:8.17.0"
checksum: 10/fa934d9fd88070833c57a3e79c0f933d0b68884c00293a1d571889b882e5c9680ccfdc5c77a7160d5a4b8b46657f93db2468a4726a517fce4d3bc764b66f1995
languageName: node
linkType: hard

"@typescript-eslint/scope-manager@npm:8.18.1":
version: 8.18.1
resolution: "@typescript-eslint/scope-manager@npm:8.18.1"
Expand All @@ -14961,39 +14933,13 @@ __metadata:
languageName: node
linkType: hard

"@typescript-eslint/types@npm:8.17.0":
version: 8.17.0
resolution: "@typescript-eslint/types@npm:8.17.0"
checksum: 10/46baf69ab30dd814a390590b94ca64c407ac725cb0143590ddcaf72fa43c940cec180539752ce4af26ac7e0ae2f5f921cfd0d07b088ca680f8a28800d4d33a5f
languageName: node
linkType: hard

"@typescript-eslint/types@npm:8.18.1, @typescript-eslint/types@npm:^8.0.0":
version: 8.18.1
resolution: "@typescript-eslint/types@npm:8.18.1"
checksum: 10/57a6141ba17be929291a644991f3a76f94fce330376f6a079decb20fb53378d636ad6878f8f9b6fcb8244cf1ca8b118f9e8901ae04cf3de2aa9f9ff57791d97a
languageName: node
linkType: hard

"@typescript-eslint/typescript-estree@npm:8.17.0":
version: 8.17.0
resolution: "@typescript-eslint/typescript-estree@npm:8.17.0"
dependencies:
"@typescript-eslint/types": "npm:8.17.0"
"@typescript-eslint/visitor-keys": "npm:8.17.0"
debug: "npm:^4.3.4"
fast-glob: "npm:^3.3.2"
is-glob: "npm:^4.0.3"
minimatch: "npm:^9.0.4"
semver: "npm:^7.6.0"
ts-api-utils: "npm:^1.3.0"
peerDependenciesMeta:
typescript:
optional: true
checksum: 10/8a1f8be767b82e75d41eedda7fdb5135787ceaab480671b6d9891b5f92ee3a13f19ad6f48d5abf5e4f2afc4dd3317c621c1935505ef098f22b67be2f9d01ab7b
languageName: node
linkType: hard

"@typescript-eslint/typescript-estree@npm:8.18.1":
version: 8.18.1
resolution: "@typescript-eslint/typescript-estree@npm:8.18.1"
Expand Down Expand Up @@ -15027,16 +14973,6 @@ __metadata:
languageName: node
linkType: hard

"@typescript-eslint/visitor-keys@npm:8.17.0":
version: 8.17.0
resolution: "@typescript-eslint/visitor-keys@npm:8.17.0"
dependencies:
"@typescript-eslint/types": "npm:8.17.0"
eslint-visitor-keys: "npm:^4.2.0"
checksum: 10/e7a3c3b9430ecefb8e720f735f8a94f87901f055c75dc8eec60052dfdf90cc28dd33f03c11cd8244551dc988bf98d1db9bd09ef8fd3c51236912cab3680b9c6b
languageName: node
linkType: hard

"@typescript-eslint/visitor-keys@npm:8.18.1":
version: 8.18.1
resolution: "@typescript-eslint/visitor-keys@npm:8.18.1"
Expand Down
Loading