#855 code cleaning not related to issue #873
Conversation
…tions spanning sequence end.
…ters more may be less readable I changed strategy and configured parser class.
…anning the end point. Location contract wants sublocations
…ger. This fix should be propagated to Location class and related.
josemduarte
left a comment
josemduarte
left a comment
There was a problem hiding this comment.
Thanks a lot for the nice cleanup! Two comments:
- this is mixing up some of the changes that are in PR #872 . Could you separate those changes out of this PR?
- some of the changes here are affecting public methods and interfaces signatures. That would normally deserve a bump of major version. However since they are not too dramatic (simply changing collection implementations to interfaces e.g. ArrayList->List) I would be ok to have them in next minor release (5.4.0). Anyone not ok with that?
Could we run a binary compatibility check using Clirr or another similar tool? The problem is not that changes are big or small, it is rather that users run may run into |
|
@heuermh The changes here would mean a compile time error for downstream users (that's why it should be a Major version bump). I was simply saying that for this case it's a minimal change since the interface is mostly conserved when switching |
|
You are right @josemduarte , I didn't put too much attention to that since I had the feeling I was not changing API exposed classes, but eventually this is not the case, sorry about that. And yes, when I started I didn't suppose to make a lot of changes and unfortunately I didn't create another branch from master. That would have prevented those changes on the top of #872 but in fact my initial purpose was just cleaning touched files. Unfortunately I don't think what you ask is a very easy task. |
|
Ok, thanks @paolopavan . Indeed, I think we can merge #872 to include it in next release (5.4.0), whilst keeping this one open and defer for next release. We are likely going to need a 6.0.0 release soon for other reasons, so we can do all these API changes together. |
|
Fair enough, @josemduarte . |
|
Ok I'll close the PR. @paolopavan please make a new one after the 5.4.0 release happens. |
3 participants
This PR contains general code cleaning not specifically related to the issue. Just a bit of improving quality of the files touched by #855, left apart to ease the review of the main issue.