-
Notifications
You must be signed in to change notification settings - Fork 38
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
base: main
Are you sure you want to change the base?
Conversation
@@ -265,6 +266,21 @@ const prepareWorkspace = (relativeDirectory = '.', projectPackageManager = 'npm' | |||
} | |||
} | |||
|
|||
if (argv.minimal) { | |||
const editorconfigPath = resolve(cwd, '.editorconfig'); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 optionminimal
set to true
Would be worse to add it as comment as well in the code
b4f1a2f
to
7b41e4e
Compare
7b41e4e
to
c22bda8
Compare
View your CI Pipeline Execution ↗ for commit c22bda8.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files☔ View full report in Codecov by Sentry. |
Proposed change
Generate eol LF config
Related issues