@luislavena meanwhile the specs are failing 😢

Seems you based your change on a previous version. Please review your
changes against master.

Sorry for top posting, sent from mobile.
On Apr 13, 2016 05:02, "Serdar Dogruyol" notifications@github.com wrote:

@luislavena https://github.com/luislavena meanwhile the specs are
failing [image: 😢]

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7 (comment)

Okay, i'll review tonight 👍

@luislavena I just checked that and can confirm that this is up to date 👍

Sorry but no, you're removing the work from master like SharedKeyError, the
types from instance variables and the fixes to 'Node#key' type.

Sorry for top posting, sent from mobile.
On Apr 13, 2016 14:21, "Serdar Dogruyol" notifications@github.com wrote:

I just checked that and can confirm that this is up to date [image: 👍]

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7 (comment)

Ah okay, i've just seen that my bad 😢

If you want i can remove this PR and wait for you to update Radix. WDYT?

Bear with me, but I'm missing the point from your pull request (will be
great to provide details that explain them)

Is there something specific you're trying to fix or that radix doesn't work
as expected?

From what I skimmed on the diff, I couldn't tell what is what you want to
achieve.

Thank you.

Sorry for top posting, sent from mobile.
On Apr 13, 2016 14:46, "Serdar Dogruyol" notifications@github.com wrote:

Ah okay, i've just seen that my bad [image: 😢]

If you want i can remove this PR and wait for you to update Radix. WDYT?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7 (comment)

As you may already know i'm using Radix for Kemal and while updating Kemal to new Crystal version( which makes instance variable types a must) i've also needed to update Radix.

For example this won't work in the new Crystal version because the type is not known https://github.com/luislavena/radix/blob/master/src/radix/result.cr#L17.

This PR is the sum of all that instance type requirements.

Note: The link to related Crystal issue crystal-lang/crystal#2443

I haven't check the new changes on Crystal, but definitely sounds like a
breaking change for Radix, thus bump of version.

In relation to payload, you cannot remove the possibility of it to be nil,
since that is used when building the tree.

That means that T also needs to consider | Nil

Right now in bed (sick) so will try to look at this maybe tomorrow or
Thursday.

Hope you understand.

Sorry for top posting, sent from mobile.
On Apr 13, 2016 14:55, "Serdar Dogruyol" notifications@github.com wrote:

As you may already know i'm using Radix for Kemal and while updating
Kemal to new Crystal version( which makes instance variable types a must)
i've also needed to update Radix.

For example this won't work in the new Crystal version because the type is
not known
https://github.com/luislavena/radix/blob/master/src/radix/result.cr#L17.

This PR is the sum of all that instance type requirements.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7 (comment)

@luislavena oh sorry to bother you. Hope you get well soon 🙏

Thank you!

@luislavena i've update the specs but can't find a solution for initializing a Tree with a Node with no payload.

Here's the stacktrace.

Error in ./spec/radix/tree_spec.cr:28: instantiating 'Radix::Tree(TestPayload):Class#new()'

        tree = Tree(TestPayload).new
                                 ^~~

instantiating 'Radix::Tree(TestPayload)#initialize()'

in ./src/radix/tree.cr:29: instance variable '@root' of Radix::Tree(TestPayload) must be Radix::Node(TestPayload), not Radix::Node(Nil)

      @root = Node(T).new("", placeholder: true)

@luislavena now all specs are passing 🚀 👍

I think this is ready for me 😄

Thank you @sdogruyol, I have fixes for this locally but need more testing against our internal codebase. Will update tomorrow.

Cheers.

@luislavena any update on this 😃

@sdogruyol my coworkers and I decided to go with changes in #8, which included the optional payload and updates to the documentation.

Going to wrap this as 0.3.0 and tag it soon.

@sdogruyol v0.3.0 has been tagged: https://github.com/luislavena/radix/releases/tag/v0.3.0

Good job 👍

Thank for your contribution and patience.

Have a nice weekend.

Thank you too 🎉