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

IDL spec: Remove unsound rules #710

Merged
merged 5 commits into from
Dec 10, 2019
Merged

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Oct 1, 2019

now that other teams are starting to look at the IDL spec we should not
confuse them by having broken rules in this document, so let’s remove
them.

Issue #364 is where we are discussing possible ways to fix them.

now that other teams are starting to look at the IDL spec we should not
confuse them by having broken rules in this document, so let’s remove
them.

Issue #364 is where we are discussing possible ways to fix them.
@nomeata nomeata added the automerge-squash When ready, merge (using squash) label Oct 1, 2019
@nomeata nomeata requested a review from rossberg October 1, 2019 06:31
@nomeata
Copy link
Collaborator Author

nomeata commented Oct 14, 2019

@rossberg ping?

@rossberg
Copy link
Contributor

Is there much risk of confusion? The disclaimer seems explicit enough.

Not too big a fan of creating more churn than necessary. Something close to these rules still is what we want, isn't it?

@nomeata
Copy link
Collaborator Author

nomeata commented Oct 14, 2019

Something close to these rules still is what we want, isn't it?

We just had a multip day discussion about this without a conclusion. I think pretending we know what we want in the end at this point is not honest. And I found it mildly confusing when scrolling through while doing the Coq proofs.

Just like I usually ask people to remove commented out “not yet working” code, why not apply the same to specifications? It’s not lost, if we really end up needing it, and a git revert or git show + copy’n’paste is cheap. It will also make reviewing an eventual PR adding this properly easier to understand.

@nomeata
Copy link
Collaborator Author

nomeata commented Nov 4, 2019

@rossberg ping? I am still in favor of keeping the IDL spec that we are already handing out to other team for implementation free of broken stuff.

@rossberg
Copy link
Contributor

rossberg commented Nov 4, 2019

Meant to look into it this week, preferably, by fixing it instead of removing. :)

@nomeata
Copy link
Collaborator Author

nomeata commented Nov 4, 2019

Ok. I will remove them at the end of the week if we have nothing fixed by then :-)

@nomeata
Copy link
Collaborator Author

nomeata commented Nov 6, 2019

(hopefully superceded by #832)

@nomeata
Copy link
Collaborator Author

nomeata commented Dec 9, 2019

Ok. I will remove them at the end of the week if we have nothing fixed by then :-)

It’s been more than a week. Can we just satisfy my OCD and not leave broken stuff around? I promise to merge master into #832 and handle the conflict :-)

@rossberg
Copy link
Contributor

I don't think it's a problem to keep todos around, to the contrary, as long as they are clearly marked as such. But if you feel strongly about it, go ahead.

Copy link
Contributor

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Instead of removing altogether, replace them with shorter TODOs to clarify that something is still missing.

@nomeata
Copy link
Collaborator Author

nomeata commented Dec 10, 2019

Instead of removing altogether, replace them with shorter TODOs to clarify that something is still missing.

Very good idea, done!

@nomeata nomeata requested a review from rossberg December 10, 2019 09:49
@mergify mergify bot merged commit 1999696 into master Dec 10, 2019
@mergify mergify bot deleted the joachim/idl-remove-unsoundness branch December 10, 2019 11:26
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants