Convert to file-scope namespaces

[MikeAlhayek]

Yes this will cause a conflict for every PR we have. However, the current PRs seems to have no activities for a while.

If any PR need to fix it's merge conflicts, simply convert the conflicted files to file-scope namespaces or use this command

dotnet format --severity=info --diagnostics=IDE0160 IDE0161

Note to reviewers, since it would be crazy to review 3.5k files, I run the following command

dotnet format --severity=info --diagnostics=IDE0160 IDE0161

Then made the changes to the following files:

• .editorconfig changed the csharp_style_namespace_declarations to warning.
• updated the PR CI workflow so it verify if the format is correct based on the preferences in .editorconfig

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[Piedone]

As with other such PRs, it only makes sense if you configure an analyzer to guard against it, and make sure it actually fails the build in CI.

[MikeAlhayek]

We already have a suggestion for this in the .editorconfig

OrchardCore/.editorconfig

Line 35 in 5c45566

csharp_style_namespace_declarations = file_scoped:suggestion

maybe we can use warn instead of suggestion

[MikeAlhayek]

@Piedone I changed the editor settings to warn against using block-scope namespaces.

[Piedone]

Please check if the CI build indeed fails.

[sebastienros]

Yes this will cause a conflict for every PR we have. However, the current PRs seems to have no activities for a while.

There is a way to prevent that, apply the same change in the files that are changed by every PR. Would take a script to do it, but it doesn't seem very hard, even more with the gh cli.

• list all PRs
• foreach PR
  • apply the same tool (and changes)
  • merge main (there shouldn't be any conflicts since the untouched files would be identical)
  • push to original PR (even on fork)

[MikeAlhayek]

Please check if the CI build indeed fails.

Seems to be passing.

[MikeAlhayek]

There is a way to prevent that, apply the same change in the files that are changed by every PR. Would take a script to do it, but it doesn't seem very hard, even more with the gh cli.

This approach sounds promising. But, I don't have much time to adventure with this approach. If you have the time, adding this script will be time saver for the PR.

[Piedone]

Please check if the CI build indeed fails.

Seems to be passing.

I mean, it should fail if there's a non-compliant file. Changing the .editorconfig doesn't guarantee you'll see the same behavior in CI.

[hishamco]

I remembered Seb against this change in the past, because it will break every PR, it would be nice to add this incrementally and new PRs

[MikeAlhayek]

@Piedone I added CI to ensure important formatting is apply on the PR CI.

[Piedone]

This would be good, but indeed with some help for PR authors, at least with a comment with guidelines under each (I think just "1. dotnet format 2. commit/push and don't be afraid that all files changed 3. merge" should be enough).

[MikeAlhayek]

@Piedone look at my last commit. I expected the build to fail since the ITask does not use file-scope namespaces.

[Piedone]

See #16539 (comment)

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[lahma]

Maybe also consider ignore revisions configuration for Git after this has been merged: sebastienros/jint@a08f452

[MikeAlhayek]

Maybe also consider ignore revisions configuration for Git after this has been merged: sebastienros/jint@a08f452

@lahma that sounds like a good idea. But I think we have to do it after we merge this PR to get the hash or the merge-squash commit so we can add it to the .git-blame-ignore-revs file. Correct?

[lahma]

Maybe also consider ignore revisions configuration for Git after this has been merged: sebastienros/jint@a08f452

@lahma that sounds like a good idea. But I think we have to do it after we merge this PR to get the hash or the merge-squash commit so we can add it to the .git-blame-ignore-revs file. Correct?

Yes, I believe you need to merge this first and then add he file when you know the exact hash in the repo.

[Piedone]

This will still be necessary, see Lombiq/.NET-Analyzers#71. The other two I don't think so, so OK to remove those.

[MikeAlhayek]

done

[sebastienros]

text=auto is supposed to do the conversion automatically. I don't think it's a good idea to force crlf.

[sebastienros]

And each extension can be forced for a specific format (text or binary)

Examples:
https://github.com/dotnet/dotnet/blob/main/src/aspire/.gitattributes
https://github.com/dotnet/dotnet/blob/main/src/aspnetcore/.gitattributes

[Piedone]

It's a Roslyn bug: dotnet/roslyn#55526

[sebastienros]

I read the comments, and they don't seem to imply there is a bug, I don't think so either. Sam correctly mentions the end_of_line should not be forced and that's it:

It also needs to remove this from .editorconfig and there won't be conflicting behaviors.

end_of_line = crlf

[sebastienros]

Mike did the changes, the CI is green. I also cloned on Linux after these changes, did a dotnet format and no files were updated.

And it's LF:

00000000: 7573 696e 6720 5379 7374 656d 3b0a 7573  using System;.us
00000010: 696e 6720 5379 7374 656d 2e43 6f6c 6c65  ing System.Colle
00000020: 6374 696f 6e73 2e47 656e 6572 6963 3b0a  ctions.Generic;.
00000030: 7573 696e 6720 5379 7374 656d 2e43 6f6c  using System.Col
00000040: 6c65 6374 696f 6e73 2e49 6d6d 7574 6162  lections.Immutab

[Piedone]

We'll see :).

[sebastienros]

There is a possibility there will be some glitches, but I think only from files that are incorrectly tagged as text vs. binary, or some which are currently cr-lf but should be lf only (web assets for instance).

I can have a look at one of the repositories where you have had the issue if you want and see if I can understand why you are getting problems.

[Piedone]

It seems to me the recommendation is to enforce the same line ending everywhere and not let the platform handle it: dotnet/roslyn#55526 (comment) I don't particularly like this, but we're actually doing that in the Lombiq repos (including end_of_line = crlf in the .editorconfig) and it works reliably, no issues since then.

But perhaps indeed if we removed all of this then it'd also work.

This was a problem with all of our projects using https://github.com/Lombiq/.NET-Analyzers, i.e. where these props and config files are loaded. Thank you!

[Piedone]

Please help PR authors though: #16539 (review)

[MikeAlhayek]

I think all the anthers needs to do is run the following command in their PRs

dotnet format --severity=info --diagnostics=IDE0160 IDE0161

Then maybe also gulp rebuild if they made any modifications to assets.

We would have to merge this one, then try the above steps on some of the PRs to see if that fixes everything or if we need to other steps.

The problem is that most of the current PRs already have merge conflicts. and trying to fix them with the above two commands may not be easy. But either way, they should start with the above commands, and they fix the per-existing merge conflict.

[Piedone]

Don't tell me, tell the PR authors :).

[MikeAlhayek]

Don't tell me, tell the PR authors :).

After we merge this, I'll attempt to fix a PR and provide steps for the others to follow.

[MikeAlhayek]

@Piedone if you are good with the last changes, please merge this PR. Otherwise, please provide more feedback.