Closed lloydzhou closed 9 years ago
This is very interesting - you managed to shrink a 160 LOC library into a 90 LOC one, preserving the functionality, and removing some functions at the same time. I like it.
I think the tests made your job more difficult than needed, because they tested things about the internal structure of the router tree. This is an implementation detail and as such it should not have been tested - only the external effects of the library should have. I will probably remove those tests from the spec. My bad.
I would like to merge this PR, but there are however a couple points where I have doubts:
resolve
function. There are too many 1-letter identifiers now, doing too much. key
is probably not a good name for the iterator variable - it's also a token. The key:byte(1) == 58
means key starts with ':'
, but that definitively needs to be more explicit. The machine understands it, but we can't expect that the humans which will maintain the code will know which char is number 58. I threw me off for a moment :)@lloydzhou : Let me know if you can do points 1 and/or 2. If not, I will do them before merging the code.
@mikz I change the test case just because i using the string key for table "router._tree". @kikito Thanks for you reply, i'll update my code.
@kikito
for i in {1..1000}; do busted;done | awk '/second/{print $13}' | paste -sd+ | bc APItools:master 11.637493s lloydzhou:master 11.138230s
@kikito do you see the last commit of this pull request?
Hi @lloydzhou ,
I saw it, and started doing some changes in my own branch. Unfortunately I have been busy with real-life and other coding projects and this has taken a back seat. It might still take me a while to get back to this -I will at least have a couple weeks in April.
Apologies for my tardiness. This will be eventually merged (with changes)
Hi @lloydzhou,
I have merged your changes after doing some changes:
Thanks and regards!
@lloydzhou hey, thanks! Could you elaborate a bit on why it is needed and what it does? Also if it breaks compatibility in some way? It changes tests, but looks like it affects only internals.