Uh oh!
There was an error while loading. Please reload this page.
Use current inbox property - #107
Conversation
melvincarvalho
commented
Sep 27, 2018
Thanks for raising this and the pointers. We have a big release on node solid server coming up. Let's revisit after that. |
dmitrizagidulin
commented
Sep 27, 2018
👍 looks good to me. Thanks @csarven! |
RubenVerborgh
commented
Sep 27, 2018
Hi @melvincarvalho, I think we should move on with this. Using the old property is an error in the documentation that we don't want to propagate. I've done a grep on the entire repository and @csarven has changed all occurrences. So I will provide a positive review on this PR and merge tomorrow, unless strong objections come up. |
RubenVerborgh
left a comment
There was a problem hiding this comment.
All occurrences have been correctly changed.
@RubenVerborgh@dmitrizagidulin really appreciate your eyes on this, and thank you! So, there's a couple of subtle points here, which I think think would be helpful go over. This has been open for ages, isnt hurting anyone, let's discuss in a week or two, if that's OK? IMHO some really good and exciting developments coming after this week, would just like to put this on hold until after, if that is ok ... |
melvincarvalho
commented
Sep 27, 2018
Addendum : please dont let me get in the way of consensus, but have you really evaluated this spec change fully? Does solid : inbox occur in any of the tests? IMHO changes to the spec are worth taking a bit of time over! But, I can live with this change :) |
melvincarvalho
commented
Sep 27, 2018
Concretely, please consider that if we want to go this route, the tests should match the proposal, as an example : |
kjetilk
commented
Sep 27, 2018
OK, thanks, @melvincarvalho ! It didn't look like a big burden on the 5.0.0 release, and I didn't realize you actually felt it was premature. Sorry I was making assumptions about your motivation rather than just asking. But yeah, we need to follow up with a bug on node-solid-server, and a fix in there. Anyone up for creating that issue? And possibly fix the bug? :-) |
RubenVerborgh
commented
Sep 28, 2018
So what I'm hearing is that this PR is correct and approved, but that there might be issues in other repositories. So let me merge this and track the rest in #108. |
melvincarvalho
commented
Sep 28, 2018
good stuff! FYI : chatted to @RubenVerborgh out of band and went over the background, we've identified some areas to cleanup, which will be covered in other issues |
As per: