Skip to content

Remove InputPin unproven flag per #41 #102

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Remove InputPin unproven flag per #41 #102

wants to merge 1 commit into from

Conversation

ryankurte
Copy link
Contributor

@ryankurte ryankurte commented Oct 11, 2018 •
edited
Loading

InputPin trait has been demonstrated (see: #41 (comment) amongst other impls), this PR removes the unproven feature flag. This is either blocked by or dependent on #95 and #97 (what do you think, @eldruin?), and blocks rust-embedded/linux-embedded-hal#11

@ryankurte ryankurte requested review from ilya-epifanov, thejpster and a team as code owners October 11, 2018 00:38
@eldruin
Copy link
Member

eldruin commented Oct 11, 2018

Thanks @ryankurte. It would be easiest for me if this was done after landing #97. I will push some changes to #97 this afternoon and I think it should be quite ready for merge then.

However, landing this would mean declaring an infallible trait proven and at the same time it is clear that we want to replace it with a fallible version.
What do you think?
An alternative could be to leave the traits behind unproven for this release and in the next release make only the fallible version proven.

@ryankurte
Copy link
Contributor Author

Ahh, you make a good point. Probably better to leave it unproven and close this and wait I suspect. Thanks for the response!

@ryankurte ryankurte closed this Oct 11, 2018
@therealprof
Copy link
Contributor

So here's the thing: Marking something as proven is a compatible change so we can easily do that anytime (and we should have a long time ago). The fallible version OTOH is a breaking change so it will require a bit more coordination (and pooling with other breaking changes into a new minor release) and it is going to be unproven (unless there's enough implementations already, to make it proven).

So for me those two topics are really separate issues and I would go ahead and make the compatible change now and the other change whenever everything is ready.

@rust-embedded/hal and all, really: Opinions?

@ryankurte ryankurte reopened this Oct 11, 2018
@hannobraun
Copy link
Member

Question: We're about to change those traits, so would that not reset their proven-ness? I mean, the new fallible version won't initially have the number of required implementations and users.

And how will we handle this in general? If we decide to handle breaking releases as clean cuts as opposed to having a transition strategy (see #100), will it ever be okay to re-add a trait to the unproven set? Seems user-unfriendly to remove a proven trait and require the user not only to upgrade, but opt into unproven. But if we discover problems in a trait after it had been in use for a while, then the fixed version of the trait is essentially unproven.

I'm not sure if that's going to be my position going forward, but this certainly is one of those moments where I'm thinking that the whole unproven concept is a useless waste of effort, as long as we're pre-1.0.

@ryankurte
Copy link
Contributor Author

Question: We're about to change those traits, so would that not reset their proven-ness? I mean, the new fallible version won't initially have the number of required implementations and users.

I think my perspective has changed and I agree with @therealprof in this instance, that this trait has been proven, and that whether we revert / introduce unproven for new breaking changes is a separate issue / decision to be made.

@therealprof
Copy link
Contributor

@ryankurte Care to check out the conflicts?

@eldruin
Copy link
Member

eldruin commented Oct 15, 2018

As I understood the discussion, implementing #97 as a clean break would mean the replacement fallible version of the InputPin would have to be unproven, right?

@ryankurte ryankurte mentioned this pull request Oct 16, 2018
Closed
@ryankurte ryankurte added the S-waiting-on-review Review is incomplete label Oct 16, 2018
hannobraun
hannobraun previously requested changes Oct 17, 2018
View reviewed changes
Copy link
Member

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

As the discussion in #92 has made clear to me, we're not at all on the same page about what's currently happening, probably due to too many discussions going on in parallel in many places. I'm officially registering my opposition to merging this pull request, unless we decide on a transition plan for the digital traits in #100.

Context: #92 (comment)

@hannobraun hannobraun added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Review is incomplete labels Oct 17, 2018
@Disasm
Copy link
Member

Disasm commented Feb 24, 2019

Ping from triage: is there any progress on this?

@hannobraun hannobraun dismissed their stale review February 25, 2019 10:04

We came up with a compromise for the transition plan, and that's already been enacted. Not sure what's supposed to happen here right now (I'd need to review the policy on unproven, which I don't have time for right now). I any case, I'm no longer opposed to this, and I'm dismissing my review.

@ryankurte ryankurte mentioned this pull request Feb 27, 2019
Merged
bors bot added a commit that referenced this pull request Mar 21, 2019
@bors @ryankurte
@eldruin @therealprof
Merge #127
264d080 
127: Digital v1 <-> v2 compatibility wrappers and shims r=japaric a=ryankurte

This PR introduces implicit  v1 -> v2 (forward) compatibility, and explicit wrapper types for v2 -> v1 (reverse) compatibility between digital trait versions as a final step for #95, as well as moving the deprecated v1 traits to a re-exported module for clarity.

As @japaric pointed out, it is not feasible to have implicit compatibility in both directions, so it seemed reasonable to make regression explicit as it hides an `.unwrap()` on failure.

@therealprof, @hannobraun, @eldruin what do you think of this approach?
I think it probably needs more documentation, though I am definitely open to suggestions as to what / where.

See also: #100, #92, #102.



Co-authored-by: Ryan Kurte <[email protected]>
Co-authored-by: Diego Barrios Romero <[email protected]>
Co-authored-by: Daniel Egger <[email protected]>
@mathk
Copy link

mathk commented May 29, 2019

Ping from triage: Can we rebase and merge ?

@mathk mathk added the S-waiting-on-author Author needs to make changes to address reviewer comments label May 29, 2019
@therealprof
Copy link
Contributor

Hm, I guess the moment we land this someone is going to request a fallible version of it. 😅

@mathk
Copy link

mathk commented May 29, 2019 •
edited
Loading

Rich Hickey has a good point saying that union type is better than class type as it avoid breaking compatibility.

@therealprof
Copy link
Contributor

Rich Hickey has a good point saying that union type is better than class type as it avoid breaking compatibility.

Must have missend that comment. Have a link to it?

@mathk
Copy link

mathk commented May 29, 2019

https://www.youtube.com/watch?v=YR5WdGrpoug

@eldruin
Copy link
Member

eldruin commented May 29, 2019

Hm, I guess the moment we land this someone is going to request a fallible version of it. sweat_smile

I already did :) I don't do it for fun, just that I want to get rid of panicking on error. At the moment I have no other choice.

@mathk
Copy link

mathk commented May 29, 2019

@eldruin I guess what @therealprof says is that someone would like the trait to have fn is_high(&self) -> Result<bool>; instead of bool.

@caemor
Copy link

caemor commented May 29, 2019 •
edited
Loading

@mathk That (Result<bool>) is what Eldruin already needs to get rid of panicking on these errors (see eldruins link above) ;-)

But since v2 of the Input Pin is already fallible that shouldn't be a problem anymore.

But is it really useful to remove the unproven flag here when v1 is already deprecated?

@ryankurte
Copy link
Contributor Author

this PR applies to a previous version of the library, so i'm going to close it as no-longer-required.

feel free to open an equivalent if you would like to mark the digital::v2 traits as proven, check out and contribute to existing discussion about the v2 traits at #135, or open a new issue for anything else ^_^

@ryankurte ryankurte closed this May 29, 2019
@ryankurte ryankurte deleted the prove-inputpin branch May 29, 2019 23:42
@nils-van-zuijlen nils-van-zuijlen mentioned this pull request Nov 14, 2019
Merged
bors bot added a commit that referenced this pull request Nov 17, 2019
@bors @nils-van-zuijlen
Merge #164
db09d73 
164: Make the digital::{v1, v2}::InputPin traits proven r=therealprof a=nils-van-zuijlen

See #41 

Previous attempt : #102 

Co-authored-by: Nils VAN ZUIJLEN <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hannobraun hannobraun hannobraun left review comments

@ilya-epifanov ilya-epifanov Awaiting requested review from ilya-epifanov

@thejpster thejpster Awaiting requested review from thejpster

Assignees
No one assigned
Labels
S-waiting-on-author Author needs to make changes to address reviewer comments S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ryankurte @eldruin @therealprof @hannobraun @Disasm @mathk @caemor