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

Marking Libgpiod v2 api as experimental #2323

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Jun 6, 2024

cc: @raffaeler @Ellerbach @pgrawehr @krwq

Relates to #2271

Microsoft Reviewers: Open in CodeFlow

@@ -13,6 +14,7 @@ namespace System.Device.Gpio.Libgpiod.V2;
/// allows callers to retrieve information about each line, watch lines for state changes and make line requests.
/// </summary>
/// <seealso href="https://libgpiod.readthedocs.io/en/latest/group__chips.html"/>
[Experimental(DiagnosticIds.SDGPIO0001, UrlFormat = DiagnosticIds.UrlFormat)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have to add this attribute to internal classes? They're not part of the API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, short answer is that this way we know if any stable API depends on any unstable API.

Long answer: when marking some of the new public APIs as experimental, the internal code we added in libgpiod started to throw warnings as we were calling experimental APIs. There are two ways of getting rid of those warnings, you either suppress them (either in the callsite or just add it to NoWarn so they are suppressed for the whole project) or you mark the caller also as Experimental, which would just bubble the warning one level up the callstack. I chose the latter as otherwise we wouldn't know if any stable public facing API depended on any nom stable public facing API, so I marked each of the classes that showed warnings Experimental all the way up till O reached public API, and only suppressed the warnings there. This way, it requires a conscious decision to depend on experimental stuff from non experimental stuff.

Copy link
Member Author

@joperezr joperezr Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance, I know that today, the only two existing public APIs that are stable which may depend on any of the new experimental codepaths are UnixDriver.Create and LibGpiod constructor, because those are the only two places where I am suppressing the warnings.

Without making the internal types experimental in order to bubble up errors on the static analysis, figuring this out would be a challenge, and may lead us to depend on experimental stuff in places where we shouldn't.

Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@joperezr joperezr merged commit 56aa097 into dotnet:main Jun 12, 2024
10 checks passed
@joperezr joperezr deleted the ExperimentalAPI branch June 12, 2024 14:45
@github-actions github-actions bot locked and limited conversation to collaborators Jul 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants