Skip to content

Comments

Add the first draft of the proposal#49

Open
froozen wants to merge 3 commits intomasterfrom
proposal
Open

Add the first draft of the proposal#49
froozen wants to merge 3 commits intomasterfrom
proposal

Conversation

@froozen
Copy link
Member

@froozen froozen commented Dec 16, 2015

This PR will serve as the thread for the discussion of the contents of this proposal.

@froozen
Copy link
Member Author

froozen commented Dec 16, 2015

The name is really bad. Should we change it to API proposal or something along those lines?

@photm5
Copy link
Member

photm5 commented Dec 16, 2015

I’d like to add that the protobuf utils will be in the second level as
well, since our rationale for not putting the protobuf things in the second
level was that library users might want to heavily change the protobuf
specifications, but the utils would stay the same.

I think both util functions in the second level should actually be in the fourth
one, since our custom Chan type kind of belongs in there. selectChannel
would be bettor off as a prism, ie:

selectingChannel :: Word16 -> Prism' Packet Packet

Additionally, you didn’t apply all the arguments to Chan – It would be Chan Packet Packet instead of Chan Packet. And I’d switch the arguments of
transform in order to be parallel to fmap.

@froozen
Copy link
Member Author

froozen commented Dec 16, 2015

I’d like to add that the [protobuf utils][1] will be in the second level as well,

I think both util functions in the second level should actually be in the fourth one, since our custom Chan type kind of belongs in there.

I don't think all code in the library has to be separated into the different levels. For me, it's just a way to create some sort of order in our implementation of the specs.
Anything falling outside that, say utility functions and types, just exists without being bound to a certain level.

All the other suggestions

Agreed! I'll change those things right now.

@froozen
Copy link
Member Author

froozen commented Dec 16, 2015

What's @lukasepple's opinion on this so far?

@sternenseemann
Copy link
Member

Maybe the code on channels should be replaced by a link to the implementation #52?

@photm5
Copy link
Member

photm5 commented Dec 24, 2015

The more parts of the proposal I implemented on lowest-two-levels, the more the level seperation seems weird... What is meant to be in the fourth level? I don’t see anything new described in there compared to level two.

@sternenseemann
Copy link
Member

Shouldn't we just merge this?

@froozen
Copy link
Member Author

froozen commented Dec 25, 2015

Level two implements the parsing of raw ByteStrings into the Packet type. This is something that probably won't change between protocol versions in the future.
Level four will implement the conversion of those Packets into more specific types, like ChatMessage. As this is very dependant on protocol specification versions, it should reside in a higher level of the API.

@photm5
Copy link
Member

photm5 commented Dec 25, 2015 via email

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.

3 participants