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

Proposals for bindings -- NRTs and C# syntax #1245

Closed
richlander opened this issue Nov 3, 2020 · 11 comments
Closed

Proposals for bindings -- NRTs and C# syntax #1245

richlander opened this issue Nov 3, 2020 · 11 comments
Labels
area-infrastructure documentation Documentation bug or enhancement, does not impact product or test code

Comments

@richlander
Copy link
Member

richlander commented Nov 3, 2020

We will soon merge #1243, which will enable nullable reference types for the repo (or large parts of it). Going forward, I think we should consider the following requirements:

  • Projects must enable nullable reference types to be accepted.
  • Projects should use single line using statements (for dispose) unless there is a reason to require scoping.
  • Sample projects should target .NET 5.0 and use top-level programs, unless there is a good reason not to. Test projects should also target .NET 5.0.
  • Name sample apps Program.cs, unless there are multiple sample projects in the same directory.

I mention these things, because they describe most of the fixes I made in #1243, with the goal of making the codebase modern and easier to read.

On a broader note, 5.0 will go out of support in ~ March 2022 and 3.1 in December 2022. It would be really nice to start using C# 9 or even C# 10 features throughout the codebase. Records would be the most obvious feature to take advantage of, particularly for all the models typically defined by bindings. I'd like to start doing that before the end of 2022 but don't want to give up on compatibility either. We should determine if there are creative ways to enable moving the codebase to use newer features.

Any thoughts on these ideas?

@richlander richlander added the bug Something isn't working label Nov 3, 2020
@pgrawehr
Copy link
Contributor

pgrawehr commented Nov 3, 2020

On a broader note, 5.0 will go out of support in ~ March 2022

Not sure what that means to us. It hasn't been officially released yet, so why should we already care about its end-of-support? I guess at least some 5.1 (or whatever it will be) shall become an LTS version?

@joperezr
Copy link
Member

joperezr commented Nov 3, 2020

regarding projects requiring nullable to be enabled, that will be a requirement anyway because iot.device.bindings will have it enabled so if this is not considered then the build will fail.
For the using statements, I agree, and we should probably drop a note for that and for nullable here: https://github.com/dotnet/iot/blob/master/Documentation/Coding-guidelines.md

I guess at least some 5.1 (or whatever it will be) shall become an LTS version

Actually next LTS will be .NET 6.0 which will be released in a year from now, so I'm fine with targeting 5.0 for most of the codebase now and switch it to 6.0 once that is ready.

@joperezr joperezr added documentation Documentation bug or enhancement, does not impact product or test code area-infrastructure and removed bug Something isn't working labels Nov 3, 2020
@joperezr joperezr added this to the Future milestone Nov 3, 2020
@hugener
Copy link

hugener commented Nov 3, 2020

Since there are already individual projects for each device binding (correct me if I'm wrong), I wonder if it wouldn't be possible to publish those as packages as well (Per device binding package)?
It would allow us to still consume these bindings, even if they are not ready to be included in Iot.Device.Bindings packages, as described as being an issue once NRT will be enabled.

@joperezr
Copy link
Member

joperezr commented Nov 3, 2020

We have considered moving into shipping individual bindings packages in the past but have resulted back to continue just shipping single package as it is much easier for maintainability, code share, and since most projects always use more than one binding most of the times. The idea we have now is that adding NRT shouldn't be a big blocker for adding new bindings (we can evaluate this in upcoming PRs once the NRT one is merged) so it shouldn't be too much of an issue to include this as part of authoring the binding.

@richlander
Copy link
Member Author

I later realized that we can start using records now. I was conflating C# versions with .NET versions, which is why I (foolishly) went into the support/EOL topic. I'll drop that whole topic. There is no problem.

It is a big step function to to shipping one binding per package. We need a better model than that. Making bindings NRT-compatible isn't hard, so I don't think it should stop anyone from contributing bindings. We can help if anyone is having trouble. I had to ask for help, too. See: https://twitter.com/runfaster2000/status/1322704504677036032.

@frogcrush
Copy link

Will this break nanoFramework support?

Just wondering because in one of my PRs I had a comment saying not to use LINQ, due to nanoFramework lacking support.

@Ellerbach
Copy link
Member

Will this break nanoFramework support?

Some will but nothing that can't be transformed automatically for nano in vast majority of the cases. While for Linq it's more complicated. The question for each binding is then: does this one is a candidate for nano or is it just for larger devices?
At this questions hats like GopiGo or BrickPi or PyJuice and few others, the answer is quite straight forward: those are done mainly for Raspberry Pi. For some others, the question is much less evident.

@richlander
Copy link
Member Author

This plan shouldn't break nano support. Generics are presumably a bigger issue than records and NRTs.

@krwq
Copy link
Member

krwq commented Nov 24, 2020

@richlander what's left here except switch statement PR?

@Ellerbach
Copy link
Member

I think we are there :-) There are few elements that broke during the hard migration work. I guess for the most complex bindings like the NFC readers and few others, most things have been corrected, some are still in PR.
We can anyway easily re open issues for specific bugs.

@krwq
Copy link
Member

krwq commented Jan 11, 2021

Closing this as fixed. If there is anything left please create new issues with specific work items

@krwq krwq closed this as completed Jan 11, 2021
@dotnet dotnet locked as resolved and limited conversation to collaborators Feb 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

No branches or pull requests

7 participants