Removes jackson-utils dependency. #19
Conversation
|
OK well, I didn't expect this PR to come in so fast! There is ONE thing however: this is a huge, all in one go commit; I'd much rather, if you could spare the time, that it be separated in smaller commits. Note that I am willing to merge it, I just cannot overlook the effort you put into it. |
|
Hi I have fixed the parts you commented on PR and I have sent a new PR. About splitting the original commit now it would be a bit tricky. I understand your point, but the only thing I have done in first commit is remove the dependency and fix all compilation errors. To not undo all changes and do it again with different commit I am going to comment the code of the PR. Moreover currently the PR can be merged without conflicts so it should be pretty easy. |
| private static final ReferenceToken LAST_ARRAY_ELEMENT | ||
| = ReferenceToken.fromRaw("-"); | ||
|
|
||
| private static final JsonPointer EMPTY = JsonPointer.compile(""); |
There was a problem hiding this comment.
Jackson JsonPointer does not have an empty method. For this reason we created an empty JsonPointer.
|
OK well, I'm ready to merge; there are other cleanups I wish to do but I'll do them after the merge. Before that... I take it you ran all tests and they run just fine? ;) |
|
Yes of course :D BTW I can open a new issue to add travis integration so in El Thu Dec 04 2014 at 12:23:14 PM, Francis Galiegue (<
|
|
Merging now and doing cleanups -- thanks a lot for the effort, I really appreciate it! Once the cleanup is done I'll try and integrate travis as you recommend (could help for my other projects too). |
|
Uh, great; found a bug in Jackson's JsonPointer! I was unfortunately right here; Jackson(s implementation is far from being as well tested as mine is! |
|
No problem I am collaborating with Jackson guys as well in JsonPointer API, so you can open an issue there or simply you can explain me and I will fix there. 👍 |
|
Well, the immediate problem we have here is that right now json-patch's master branch is non functional! The problem is quite simple really: try and Ideally what should be done is a full port of my implementation in the meanwhile. At least I know this one works fine and is tested! |
|
I will investigate and fix it :) I think that we can create a branch with BTW then we need to create more tests on json-patch, I mean a test for El Sat Dec 06 2014 at 7:10:20 PM, Francis Galiegue (<
|
|
Yeah, one solution would probably be to extend the class (since it is not final; I really wonder why, such a class should basically be immutable)... Do you plan to work on this? While I can do it, I also have other stuff to work on ;) |
|
OK, well, sorry but I have reverted the master branch to before the merge; I won't merge it until the JsonPointer problem is fixed. I have created the topic/noguava branch, so you will want to work on this branch instead. Note that I have fixed a bug in your pull request... You didn't seem to have run all the tests since the JSON Patch test suite failed; this is because (Jackson's) JsonPointer didn't deserialize correctly. I have fixed that. |
|
I think it has sense this approach. BTW I thought I run the whole test suite, but probably because I am not an expert of Gradle I run the incorrect goal. |
|
Provided no nagging bug still lies around... A good idea would be to "port" the test suite of JsonPointer in jackson-coreutils (which is very, very complete) to the one of Jackson. Hopefully no other bugs will be found ;) |
|
In particular, this one: Will Jackson do the correct thing here? Not sure ;) |
|
Really good idea I will move this test there :)
|
In order to remove Google Guava from as dependency, we thought that instead of removing from jackson-utils and then adapt the code of two projects, it could be better to update to Jackson 2.4 which already implements JsonPointer.
We know that this change is a big change and the leader of the project may decide to reject the PR because of this.
For our case and for our business it fits perfectly to use Jackson 2.4,but we fully understand that this it may not be perfect for other cases.
Anyway we decided to send a PR because maybe the leader of the project may find it useful and can be accepted or do some code review to make it happens on upstream project. If not and following the ASL2.0 license we modified the code and made it available on our public github for anyone who wants to use it.
As I said our main intention is to merge this change to upstream repo.