search

sanand0 / xmljson

xmlsjon converts XML into Python dictionary structures (trees, like in JSON) and vice-versa.
MIT License
121 stars 33 forks source link

resolved issue#21 #22

Closed mukultaneja closed 7 years ago

sanand0 commented 7 years ago

Thanks @mukultaneja -- could you please introduce the test cases as discussed?

mukultaneja commented 7 years ago

Hi Anand, I have added a test case in the existing test_data method for Abdera Convention.

sanand0 commented 7 years ago

@mukultaneja - the Travis checks are failing. Could you please take a look?

@mattgd - this is a PR aimed at the issue #21 you'd raised. Thought I'd loop you in.

mattgd commented 7 years ago

@sanand0 Thanks for the heads up, and @mukultaneja, thanks for fixing this issue. I appreciate it.

mukultaneja commented 7 years ago

@mattgd thanks.

mukultaneja commented 7 years ago

Hi @dagwieers, I am looking into the issue that why Travis CI build is failing. After looking into the test report https://travis-ci.org/sanand0/xmljson/jobs/247079647, I found that two existing test cases (1 for Abdera and 1 for Cobra) are failing, would you mind if I dig more into the code to resolve this build failing issue?

dagwieers commented 7 years ago

@mukultaneja The tests were working fine before. Travis CI They only started to fail with your commit, so I expect the failures to be caused by your changes. We will not accept your changes if it fails te Travis CI tests.

mukultaneja commented 7 years ago

thanks, @dagwieers I am looking into change and update it so that Travis CI tests will pass.

mukultaneja commented 7 years ago

Hi @sanand0 @dagwieers @mattgd,

I have updated the code for my pull request so that it could pass all the test cases. I have added more nested test cases as well to validate my new changes.

I would request all of you to take a look at the changes and the test cases once. Please let me know if they are correct or need some modifications.

dagwieers commented 7 years ago

I would like to get feedback from @sanand0 as he wrote the original code.

sanand0 commented 7 years ago

I'm perfectly fine with this @dagwieers - this affects only the Abdera convention that you'd authored, so if you're OK with the test cases and the change, I'm perfectly happy to merge. It's a minimalist change, so that's great too.

Are the revised test cases in line with your expectation on the Abdera convention?