Allow typed tokens of the Tavor format to be defined outside from the parser #85
Conversation
yblein
force-pushed
the
external-typed-tokens
branch
from
d505d07
to
21f1921
Compare
| type CreateTyped func(argParser ArgumentsParser) (Token, error) | ||
|
|
||
| // typedLookup is a mapping from typed token names to their creation functions | ||
| var typedLookup = make(map[string]CreateTyped) |
There was a problem hiding this comment.
Let's be more specific here since this is a global variable, please name this "typedTokenLookup"
|
Ok so I think this is pretty good looking :-) Please attend to the comments. |
|
I'm not quite familiar with the PR system of Github yet. At this point, in order to fix the code according to your remarks, should I overwrite this commit or make new commits? |
|
Squash them in the current commit. The comments should then marked as "resolved" when their code reference changes. |
|
If you like you can change the documentation too https://github.com/zimmski/tavor#extend-typed-tokens but I can also put that on my list. |
yblein
force-pushed
the
external-typed-tokens
branch
2 times, most recently
from
772b4b6
to
de600d5
Compare
| @@ -0,0 +1,59 @@ | |||
| package token | |||
There was a problem hiding this comment.
The issue closed itself, sorry. Re-opening here.
"NewTyped, ListTyped, RegisterTyped, typedLookup, CreateTypedFunc and ArgumentsTypedParser" sounds coherent.
However I'm still wondering if this shouldn't move to a new token/typed package.
- An almost empty package is not that bad if it permits a clean API;
- The functions are going to get mixed up with the whole token package in godoc and this will get worse when adding e.g. the token attributes.
There was a problem hiding this comment.
The argument for the empty package is very true. The argument about the functions is also true, but godoc is already too crowded when it comes to the token package. Do not get me wrong, my API-mind also screams to put this into token/typed but the Go-way is a bit different. Have a look at the io package. It seems also crowded. So I think it is OK to put this into token since it resides in its own file there. And I rather have another token specific API in the token package than a pattern breakage.
There is however a problem with the token package. I put some things there that should be somewhere else, e.g., the parser stuff. I need to refactore the package but that is another issue and does not concern your PR.
Allow typed tokens of the Tavor format to be defined outside from the parser.
yblein
force-pushed
the
external-typed-tokens
branch
from
29745d6
to
9bec8ff
Compare
|
I believe I fixed everything according to your remarks. We can move token/typed.go to a new package later if needed. I also updated the README. |
|
I will take another look. Sorry for taking so long I received a big amount of mails in the last couple of days because of the GSoC. Since the deadline has passed, it finally slowed down 👍 |
| > **Note:** The mentioned implementation inconveniences will be addressed in future versions of Tavor. | ||
| Typed tokens provide additional types for formats. It is possible to define new typed tokens by calling the [`token.RegisterTyped`](https://godoc.org/github.com/zimmski/tavor/token#RegisterTyped) function. It is only necessary to implement the [Token interface](https://godoc.org/github.com/zimmski/tavor/token#Token), since typed tokens behave like regular tokens. Arguments for the typed tokens are used as initialization values for the instanced token. It is therefore not possible to lookup argument values after the typed token definition is processed. | ||
| An example of typed token creation function can be found in [the sequence token](/token/sequences/sequence.go#L29). Currently, the arguments of a typed token are limited to integers. To support new argument types, it is necessary to extend the [ArgumentsTypedParser](https://godoc.org/github.com/zimmski/tavor/token#ArgumentsTypedParser) interface and [its implementation](/parser/typed.go). To add token attributes to typed tokens, please have a look at the [token attributes section](#extend-token-attributes). |
There was a problem hiding this comment.
I am wondering if it is a good idea to put a reference to a line number in the documentation but since this is an init function I do not know a way to do this with godoc.
Allow typed tokens of the Tavor format to be defined outside the parser
|
This looks superb, well done! Your argumentation was also very sharp during the issue planing and the PR. I am looking forward to see more :-) |
Types tokens are now in charge of parsing their arguments and registering themselves.
I'm wondering if token/types.go should be moved to a separate package
token/typedso thattoken.RegisterTypedbecomestyped.Register.