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.

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
Open

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

wants to merge 1 commit into from

Conversation

matthieu-crouzet
Copy link
Contributor

Proposed change

Generate eol LF config

Related issues

@matthieu-crouzet matthieu-crouzet requested a review from a team as a code owner December 19, 2024 13:30
@@ -265,6 +266,21 @@ const prepareWorkspace = (relativeDirectory = '.', projectPackageManager = 'npm'
}
}

if (argv.minimal) {
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

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

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

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

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).

@@ -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

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

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

Copy link

nx-cloud bot commented Dec 20, 2024

View your CI Pipeline Execution ↗ for commit c22bda8.

Command Status Duration Result
nx run-many --target=test-int ✅ Succeeded 55m 51s View ↗
nx run-many --target=build --projects=eslint-pl... ✅ Succeeded <1s View ↗
nx run-many --target=test-e2e ✅ Succeeded 1m 52s View ↗
nx run-many --target=publish --nx-bail --userco... ✅ Succeeded 36s View ↗
nx run-many --target=build ✅ Succeeded 14m 24s View ↗
nx affected --target=lint --base=remotes/origin... ✅ Succeeded 14m 28s View ↗
nx affected --target=test --cacheDirectory=D:\a... ✅ Succeeded 10m 6s View ↗
nx affected --target=test --cacheDirectory=/hom... ✅ Succeeded 7m 48s View ↗
Additional runs (3) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2024-12-20 08:04:49 UTC

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.96%. Comparing base (0a97daf) to head (c22bda8).

✅ All tests successful. No failed tests found.

Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: @o3r/create should generate end-of-line LF config
4 participants