Codebase modernization

[davidnmbond]

So, as @robertmclaws has noted, the (erstwhile) fork gained significant modernisation changes. I propose:

• resolving outstanding pull requests
  • voting to accept/reject ALL existing PRs (my default will be to reject unless a strong case is made, to avoid regressions)
  • merging in pull requests where a majority (who express an opinion within a week) agree (split votes mean no action)
• at this point we have a stable place from which to apply a huge swathe of changes that will affect the whole codebase
• modernization steps
  • bumping unit tests up to NET 6.0 edit: where possible
  • applying nuget package updates to all non-library code
  • applying all ICE code recommendations
  • move to file-scoped namespaces
  • edit: proposal withdrawn: opinion is against "use global usings"

[object]

I just approved and merged 3 straightforward pull requests. I only re-ran unit tests for them to ensure they are green.
I also updated Microsoft.OData.Core and Microsoft.Data.OData NuGet packages.
Pushed the changes and released 5.26.0.

Now it's your turn people :-)

[davidnmbond]

Thanks for the release, @object. Already updated into three of my projects!

Unit tests all passing on my machine also except for the web tests against http://va-odata-integration.azurewebsites.net/odata/open. It seems to be down for me. Any thoughts on that?

Can we consider you to have rejected the other PRs? This would give us a clean slate for the modernization. In the meanwhile, I will branch master to a modernization branch and continue on that.

[object]

Yes, I will reject other PRs.

[robertmclaws]

I disagree with file-scoped namespaces and global usings. Those changes are not necessary for modernization, and are usually the first thing I turn off in a VS2022/.NET 6.0 project because they've caused problems on every solution I've upgraded.

[object]

I wonder what kind of problems file scoped namespace caused?

I haven't been using file scoped namespaces myself because I am no longer in C#, but they save some identation and couple of curly brackets. I understand that this is the change that affects every file so it's worth being careful about introducing it but doesn't it make the code slightly more compact?

[davidnmbond]

My experience of file-scoped namespaces is that they make the code cleaner, easier to read, reduce indentation and are easy to apply. I have set this style in the .editorconfig (as applied to the modernization branch) and all modern IDEs will respect that for code style and apply the changes for you automatically.

I do agree, however, that global usings can be problematic and withdraw my proposal to apply these. I will apply strikethough to any comments of mine where I propose that.

[object]

@robertmclaws it looks like majority of people here agree that file scoped namespaces make code (slightly) cleaner and more compact. And global using proposal is retracted. Do you want to reconsider in favor of scoped namespace?

[object]

Still awaiting for response from @robertmclaws regarding file scoped namespaces, but to be honest if I still was a sole maintainer and received a PR from @davidnmbond with this change, I would approve it without hesitation. IMHO such change is in the mood of this library. In general I am all for making code more compact if it does not come at a cost of clarity (and this is why I am against global using that also make code more compact but less clear).

[robertmclaws]

Sorry about that @object.

For me personally, braces are containers, and statements end in a semicolon. Referencing a namespace with a using is a single statement of intent ("I need this"). Putting code inside a container is an "act".

File-scoped namespaces now turn containers into statements. That confuses intent. It also assumes developers are good about keeping files themselves scoped to a single class or namespace, and I have seen super shitty code that crams a ton of code across classes and namespaces into a single file.

I'm a big fan of clarity of intent. Having a visual connection to the resources a file is using, and the containers it creates, is important for readability. Global usings and containers-as-statements sacrifices readability for 2 characters and some indents to appease script kiddies in their transition to C#. If I had been a PM at Microsoft I would have killed both of those features.

Just my $0.02. I have no intention of holding up the team or the process with this opinion though, just to be clear.

[davidnmbond]

You LITERALLY caught me just about to apply that change! I won't apply it for now...

I think file-scoped namespaces are less controversial. Perhaps just those? I'll create a branch off the modernization branch so you can have a look.

[robertmclaws]

I mean, I don't personally think there is any point in spending the effort on that, it makes no discernable difference on the codebase. The only reason those features exist is to make C# feel more like PHP or Go. The end result is just two fewer brackets per file. 🤷‍♂️

[davidnmbond]

It's all done and there is a pull request. We also gain readability by avoiding the indentation.

[davidnmbond]

There is a modernization pull request. It's entirely non-functional changes (except a NotSupportedException is now thrown in the constructor, not the metadata fetch when the FTP protocol is requested). We can do it with or without the file-scoped namespaces pull request (into modernization).

[NetTecture]

I personally am in favour of file scoped namespaces - the lesser indentation makes things easier to read. Global using - not so much. Too much possible interference.

[object]

I second that.

I am for scoped namespaces for the reasons @NetTecture wrote. I am against global using for the following reasons:

1. Hiding a list of namespaces that a source file uses makes it obscure. I tend to like seeing a list of what code depends on, sometimes I am surprised to see a certain namespace in the list and investigate why it's there and maybe change something.
2. Global namespaces are a bit like global variables. Don't like exposing something to everything. Prefer being explicit.
3. Opening namespaces unconditionally for all files in the project may cause naming conflicts
4. Not an important reason, more esthetical one. "global" is added to some arbitrarty selected file.

[object]

It's interesting that the very first suggestion caused several disagreements. This is exacatly why I wanted a group of maintainers, not a single person. I think it what we need is principles to resolve such issues, especiall style-related because they don't provide any functional value and therefore subjective (and harder to settle).

You don't have to agree with me (I am no longer a sole maintainer) but my 2 cents is that for non-functional change, especially if it affects a large set of files, there must be approval from all main maintainers before they are applied. I hope nobody here is stubborn, so we are able to change our opinions. But being a bit conservative regarding such non-functional changes can save us from some later regrets and potentially loss of someone's motivation.

[davidnmbond]

Having more maintainers should help us get to the "right" answer!

[davidnmbond]

For those who have not yet used it, the .editorconfig file is the best way to consistently apply code style. At present, it is set to provide advisories. Later on, we should increase these to warnings (or even errors), to ensure that incoming pull requests are forced to adhere to the "house style".

I believe I have set "cleanest style" settings, though am happy to modify these.

Please review (ideally in Visual Studio Community 2022, which is free for Open Source project like this).

[davidnmbond]

Any final objections before this is merged into master?

[object]

I am fine with that.

[davidnmbond]

Done