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

RFC for the layered RPC system #30

Merged
merged 4 commits into from Sep 21, 2015

Conversation

Projects
None yet
5 participants
@SirVer
Contributor

SirVer commented Sep 12, 2015

@SirVer

This comment has been minimized.

Show comment
Hide comment
@SirVer

SirVer Sep 12, 2015

Contributor

@lastorset ping - as requested on my blog. Very interested in your comments.

Contributor

SirVer commented Sep 12, 2015

@lastorset ping - as requested on my blog. Very interested in your comments.

@gchp

This comment has been minimized.

Show comment
Hide comment
@gchp

gchp Sep 12, 2015

Here's the link to the rendered version A little easier to read.

gchp commented Sep 12, 2015

Here's the link to the rendered version A little easier to read.

@mmatyas

This comment has been minimized.

Show comment
Hide comment
@mmatyas

mmatyas Sep 13, 2015

Contributor

I have some questions:

  • Is 16 bits really necessary for priority?
  • In case some plugins have the same priority, how are they sorted?
Contributor

mmatyas commented Sep 13, 2015

I have some questions:

  • Is 16 bits really necessary for priority?
  • In case some plugins have the same priority, how are they sorted?
@mkpankov

This comment has been minimized.

Show comment
Hide comment
@mkpankov

mkpankov Sep 13, 2015

@mmatyas

My thoughts on 16 bits for priority: I think it makes sense. In edge case you'd like each plugin to have distinct priority. And, say, 8 bits is not so much in this case: I currently have Emacs setup with nearly a hundred packages, so I can imagine somebody has > 255 of them. 16 bits is much more future-proof IMO, especially in case there would be an actual Swiboe server running remotely and servicing multiple users (not sure if that's in current design though).

@mmatyas

My thoughts on 16 bits for priority: I think it makes sense. In edge case you'd like each plugin to have distinct priority. And, say, 8 bits is not so much in this case: I currently have Emacs setup with nearly a hundred packages, so I can imagine somebody has > 255 of them. 16 bits is much more future-proof IMO, especially in case there would be an actual Swiboe server running remotely and servicing multiple users (not sure if that's in current design though).

@SirVer

This comment has been minimized.

Show comment
Hide comment
@SirVer

SirVer Sep 13, 2015

Contributor

@mmatyas Those are very good questions. I posted some clarifying thoughts in 6db81db. Mostly, my thoughts mirror @mkpankov's.

The idea of priorities came from UltiSnips. Before priorities, the order of when your snippets were loaded defined which snippet overwrote which other. This was problematic, because of lazy-loading of plugins, reversed orders of plugin path traversal for pathogen and vundle. now every snippet has a priority and order is clear.

Contributor

SirVer commented Sep 13, 2015

@mmatyas Those are very good questions. I posted some clarifying thoughts in 6db81db. Mostly, my thoughts mirror @mkpankov's.

The idea of priorities came from UltiSnips. Before priorities, the order of when your snippets were loaded defined which snippet overwrote which other. This was problematic, because of lazy-loading of plugins, reversed orders of plugin path traversal for pathogen and vundle. now every snippet has a priority and order is clear.

@mmatyas

This comment has been minimized.

Show comment
Hide comment
@mmatyas

mmatyas Sep 13, 2015

Typo: define -> defined

Typo: define -> defined

@mmatyas

This comment has been minimized.

Show comment
Hide comment
@mmatyas

mmatyas Sep 13, 2015

Contributor

Ok, thanks for the explanation! What I'm afraid of is that many plugin creators may simply leave priority on default, and if that may cause problems. I wonder if you've got similar issues during working on UltiSnips?

Contributor

mmatyas commented Sep 13, 2015

Ok, thanks for the explanation! What I'm afraid of is that many plugin creators may simply leave priority on default, and if that may cause problems. I wonder if you've got similar issues during working on UltiSnips?

@SirVer

This comment has been minimized.

Show comment
Hide comment
@SirVer

SirVer Sep 14, 2015

Contributor

I think leaving priorities at 0 is fine for most plugins anyways, so I am not worried about that. It is important that there is something that can be tweaked if there are collisions.

In UltiSnips I had one or two bug reports of people not getting the priority system, but none where it made problems. I think for the RPCs, priorities are a bit more intuitive than for snippets, so I think that will work out too.

Contributor

SirVer commented Sep 14, 2015

I think leaving priorities at 0 is fine for most plugins anyways, so I am not worried about that. It is important that there is something that can be tweaked if there are collisions.

In UltiSnips I had one or two bug reports of people not getting the priority system, but none where it made problems. I think for the RPCs, priorities are a bit more intuitive than for snippets, so I think that will work out too.

@SirVer

This comment has been minimized.

Show comment
Hide comment
@SirVer

SirVer Sep 17, 2015

Contributor

Seems like there are no more comments on this RFC. I'll leave it open for now and will merge that in on the weekend if there are no more comments.

Contributor

SirVer commented Sep 17, 2015

Seems like there are no more comments on this RFC. I'll leave it open for now and will merge that in on the weekend if there are no more comments.

@SirVer

This comment has been minimized.

Show comment
Hide comment
@SirVer

SirVer Sep 21, 2015

Contributor

@homu r+

Contributor

SirVer commented Sep 21, 2015

@homu r+

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Sep 21, 2015

Contributor

📌 Commit 5e6477c has been approved by SirVer

Contributor

homu commented Sep 21, 2015

📌 Commit 5e6477c has been approved by SirVer

@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Sep 21, 2015

Contributor

⌛️ Testing commit 5e6477c with merge fc308e0...

Contributor

homu commented Sep 21, 2015

⌛️ Testing commit 5e6477c with merge fc308e0...

homu added a commit that referenced this pull request Sep 21, 2015

Auto merge of #30 - SirVer:rfc000_rpc, r=SirVer
RFC for the layered RPC system

[Rendered version](https://github.com/SirVer/swiboe/blob/rfc000_rpc/doc/rfcs/000_layered_rpc.md)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/swiboe/swiboe/30)
<!-- Reviewable:end -->
@homu

This comment has been minimized.

Show comment
Hide comment
@homu

homu Sep 21, 2015

Contributor

☀️ Test successful - status

Contributor

homu commented Sep 21, 2015

☀️ Test successful - status

@homu homu merged commit 5e6477c into swiboe:master Sep 21, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@SirVer SirVer deleted the SirVer:rfc000_rpc branch Sep 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment