-
Notifications
You must be signed in to change notification settings - Fork 585
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
Update samples to .NET 5.0 #1224
Conversation
@@ -8,12 +8,11 @@ namespace force_sensitive_resistor | |||
{ | |||
class FsrWithCapacitorSample | |||
{ | |||
private GpioController _controller; | |||
private GpioController _controller = new(); |
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.
Nifty 😄 I wasn't aware of this C#9 feature
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 know that this is just a sample but given you are touching this line we might as well just add the readonly to _controller 😆
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.
done
|
||
namespace force_sensitive_resistor |
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 understand we are trying to showcase functional programming for c#9 here, but I wonder if we should stick with object oriented language given that these samples are supposed to be entry-level sort of examples?
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.
Also, I'm open for discussion here, if you think it's best to keep it as you have it I'm fine too.
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.
Seems like keeping that line would significantly reduce the value of top-level projects. Also seems like we're not losing much. I also don't think we're losing anything OO. It would be nice if you could declare the namespace with a single line that didn't require nesting the rest of the file.
I guess we don't have any of the samples in which the Readme tells you how to run it in Windows right? Because if we do, I think it would be good if in that sample we instead targeted net5.0-windows10.0.17763.0 in order to show folks how to use our library from the Windows side. |
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.
Left some minor comments but this looks great otherwise. Thanks @richlander
I could provide a binding that reads CPU temperatures, Fan speeds etc. for Windows (using an external library). This one is really Windows only (without actually needing the GPIO stuff, though) and is also a good example for the usage of units. Want to take a look? |
While it would be good to have either that sample or a pointer to it somewhere in the repo, I think I would like to keep the samples section of the repo to specific usages of System.Device.Gpio. The idea with my question was to have one of these samples to target Windows, so having the right TFM to target that. |
I responded to the feedback and found some more opportunities for simplification (and uniformity) with dispose/using. I took an initial take at documenting windows support. We probably need a central document along the lines of |
* Update samples to .NET 5.0 * Update per feedback * Resolve conflicts * Remove duplicate project file * Update NRT errors
* Update samples to .NET 5.0 * Update per feedback * Resolve conflicts * Remove duplicate project file * Update NRT errors
* Update samples to .NET 5.0 * Update per feedback * Resolve conflicts * Remove duplicate project file * Update NRT errors
I think we can merge this at any time. We can wait until 11/10 if people want, but that doesn't seem necessary given the "go live" nature of the release.
Fixes #1190