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

Unify counts of different types #6

Open
hansihe opened this issue Mar 29, 2016 · 7 comments
Open

Unify counts of different types #6

hansihe opened this issue Mar 29, 2016 · 7 comments

Comments

@hansihe
Copy link
Member

hansihe commented Mar 29, 2016

Currently several different types require a count (buffer, pstring, array).

In protodef the counts of these vary slightly from type to type. In one you can only specify a prefix (pstring), and in the others you can specify either a fixed number, prefix type, or field reference. In these two different properties are used (count, countType).

I propose merging all these into a single property, count. I belive this would both require less mental overhead (less different types of count), and reduce the complexity of the implementation (I don't know how node-protodef does it, but in elixir-protodef I unify them in the first preprocess step to avoid this complexity in the later stages of the compiler).

Here are some examples of how I would imagine it looking like:

  • count: ["fixed", 12]
  • count: ["field", "../action"]
  • count: ["prefix", "varint"]

I am obviously open to input on this syntax.

@rom1504
Copy link
Member

rom1504 commented Mar 29, 2016

Yeah something like that would definitely make sense. Related ProtoDef-io/node-protodef#35

@hansihe
Copy link
Member Author

hansihe commented Mar 29, 2016

Ah, didn't see that. That's very closely related. Are there any plans to get it implemented?

@rom1504
Copy link
Member

rom1504 commented Mar 29, 2016

ProtoDef-io/node-protodef#35 talks of a general "selector" concept that might also apply to compareTo.
This might be interesting here.

The main difficulty I see is that countType implies adding the count type before the array/buffer, where other counts don't imply that.
That might not be a problem though, hard to say without implementing it.

@hansihe
Copy link
Member Author

hansihe commented Mar 29, 2016

The syntax I suggested very strongly implies the type being prefixed to the type.

@roblabla
Copy link
Contributor

roblabla commented Apr 2, 2016

I like hansihe's syntax better than mine. The idea is that any field that expects a "field" can either get a "fixed", "field path" or (if unremoved) "type". I don't think it'd be hard to implement countType that way, the code should more or less stay the same.

@hansihe
Copy link
Member Author

hansihe commented Apr 2, 2016

Since everyone seems to agree that this is a good idea, should we come up with some kind of plan for making the change and updating the protocols that use it? Are there any other changes we should do at the same time?

@rom1504
Copy link
Member

rom1504 commented Apr 2, 2016

Yes, things that need to be done:

  • update ProtoDef doc and tests
  • update node-protodef and elixir-protodef
  • update all .json

I believe the first step is actually updating one minecraft .json to make sure it makes sense. Then making a little script to do the transformation of the rest of them.

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

No branches or pull requests

3 participants