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

Code modernization in System.Linq #110910

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Code modernization in System.Linq #110910

wants to merge 2 commits into from

Conversation

roji
Copy link
Member

@roji roji commented Dec 23, 2024

Here are three relatively impactful code cleanups, done automatically and split into three separate commits for easier reviewing/undoing etc.

@xtqqczze
Copy link
Contributor

This might be too big to review. How about merging the most impactful change first, i.e. IDE0161: Convert to file-scoped namespace.

@stephentoub
Copy link
Member

stephentoub commented Dec 23, 2024

Use file-scoped namespace declarations in System.Linq

The repo doesn't use file-scoped namespaces; I don't think we want to change the style per library.

@eiriktsarpalis
Copy link
Member

Use file-scoped namespace declarations in System.Linq

The repo doesn't use file-scoped namespaces; I don't think we want to change the style per library.

@stephentoub do we want to consider switching the repo to file-scoped namespaces in the future? Would be nice if we could save ourselves one tab of indentation.

@roji
Copy link
Member Author

roji commented Dec 23, 2024

This might be too big to review.

@xtqqczze note that each change is a distinct commit, that should help.

@EgorBo
Copy link
Member

EgorBo commented Dec 23, 2024

Use file-scoped namespace declarations in System.Linq

The repo doesn't use file-scoped namespaces; I don't think we want to change the style per library.

@stephentoub do we want to consider switching the repo to file-scoped namespaces in the future? Would be nice if we could save ourselves one tab of indentation.

afair some *.cs files have multiple namespace scopes in the same file?
I guess such a massive change will impact git blame experience (and, well, put all active PRs into a "in-conflict" state 😆)

@stephentoub
Copy link
Member

Use file-scoped namespace declarations in System.Linq

The repo doesn't use file-scoped namespaces; I don't think we want to change the style per library.

@stephentoub do we want to consider switching the repo to file-scoped namespaces in the future? Would be nice if we could save ourselves one tab of indentation.

afair some *.cs files have multiple namespace scopes in the same file? I guess such a massive change will impact git blame experience (and, well, put all active PRs into a "in-conflict" state 😆)

It also makes servicing much harder.

@roji
Copy link
Member Author

roji commented Dec 23, 2024

Thanks for the reviewing. Have removed the file-scoped namespaces, and corrected the other stuff.

@roji
Copy link
Member Author

roji commented Dec 23, 2024

It also makes servicing much harder.

Yeah, FWIW in EF and Npgsql we try to do this kind of thing (and general large code cleanups) before we lock down a major release, to make servicing easier. It's true file-scoped namespaces do make git blaming slightly harder - I personally do think it's worth prioritizing code niceness with them, and worth the one-time pain (but I get that it's non-trivial and removed that commit).

@stephentoub
Copy link
Member

before we lock down a major release, to make servicing easier

Can you elaborate?

@roji
Copy link
Member Author

roji commented Dec 23, 2024

Can you elaborate?

Just that before a release is locked down, we usually do an automated code cleanup, to align code to the repo settings in .editorconfig and similar; this fixes any violations that have accumulated over the year from contributions where some rule wasn't respected. The timing - just before locking down the major version - is specifically done to make servicing easier, compared to e.g. if we did the cleanup right after the cleanup, at which point any backport would risk a conflict because of styling etc.

@stephentoub
Copy link
Member

The timing - just before locking down the major version - is specifically done to make servicing easier, compared to e.g. if we did the cleanup right after the cleanup, at which point any backport would risk a conflict because of styling etc.

Thanks. dotnet/runtime invariably has multiple release branches active at any one time. That complicates this, eg when locking down the .NET 10 release, we'll still have release/8.0 and release/9.0 active for another 6-12 months.

@roji
Copy link
Member Author

roji commented Dec 23, 2024

@stephentoub EF has the same servicing policy, with the multiple release branches - and it's indeed not ideal... Though the number of patches that go into older releases is typically very small compared to the number that goes into the just-released version (for example, there's typically a larger number of backports needed just after a major release, but far fewer that needs to go back even further).

Not necessarily trying to argue for anything here - just sharing info.

@eiriktsarpalis
Copy link
Member

Should we perhaps open an issue tracking the introduction of file-scoped namespaces? Might be easier to invite debate from the wider team.

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.

5 participants