Skip to content

Conversation

@AntoineLemaire
Copy link
Contributor

Fix #516

@willdurand
Copy link
Member

Well, the index already provides the level. Why not using it?

@Nyholm
Copy link
Member

Nyholm commented Oct 20, 2016

Oh, I did also miss that. Sorry-

@AntoineLemaire
Copy link
Contributor Author

It's what I explained in my issue. Why toArray function result could not be used directly by fromArray function without modifications of this array?

@willdurand
Copy link
Member

Ah. Did not read the whole issue sorry... So, does it work better now? It is a simple addition, so if you add a test case for this, then it's good to me!

@AntoineLemaire
Copy link
Contributor Author

AntoineLemaire commented Oct 20, 2016

How do you see that? Add testToArray() to all Provider tests files? Or choose a random provider in AddressFactoryTest and test toArray, fromArray and assertSame on init address and result address?

@willdurand
Copy link
Member

Is there any test class for the Address class?

@AntoineLemaire
Copy link
Contributor Author

No, only AddressFactory

@willdurand
Copy link
Member

Then, maybe you could create this test class

@willdurand
Copy link
Member

Can you also provide a test for this toArray/fromArray thing please?

@AntoineLemaire
Copy link
Contributor Author

Done & commits squash

@willdurand
Copy link
Member

Looks good to me!

@vicchi
Copy link
Member

vicchi commented Oct 22, 2016

+1 LGTM

@Nyholm Nyholm added this to the 4.0.0 milestone Dec 22, 2016
@Nyholm
Copy link
Member

Nyholm commented Dec 22, 2016

Hey @AntoineLemaire.

Sorry for the delay with this PR. If you solve the merge conflicts I'll be happy to merge

@Nyholm
Copy link
Member

Nyholm commented May 20, 2017

Fixed by #622

@Nyholm Nyholm closed this May 20, 2017
@AntoineLemaire
Copy link
Contributor Author

Oh sorry, I missed that! Thanks for merged it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants