-
Notifications
You must be signed in to change notification settings - Fork 1.1k
GH-6375: Harmonize package structures #10459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is not too hard task moving classes from one package to another with respective I have changed the comment in the issue to checkboxes, so we may even divide those packages between us. I decided to go with a Any feedback welcome! Thanks |
I think that's enough for now. Please, let me know if this is OK with you and I'll continue to nail the issue. Thanks |
So, the PR is getting bigger and bigger. I'm going to push more since I got this spare time and it makes sense for the current major we are about to finish. Thank you for your time since I feel guilty to waste it on this huge PR review 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a PR! 😆
Great work! Breaking up the inbound and outbound classes into their own packages makes perfect sense.
Just my usual nitpicks.
...ion-stream/src/main/java/org/springframework/integration/stream/event/StreamClosedEvent.java
Show resolved
Hide resolved
public void doWithMessageInternal(WebServiceMessage message, Object payload) throws IOException { | ||
} | ||
|
||
super(destinationProvider, webServiceTemplate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm debating... Do we need tests on the constructors in the deprecated classes?
My hyper nitpicky voice in the back of my head said we should at least mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not find yet a decent reason in my guts to cover deprecated classes with tests.
I treat deprecated API from my dependencies as obsolete, therefore it might be broken.
So, I switch to a new API as soon as possible.
But I may consider to have coverage for deprecated as well if you give me more arguments.
Plus, I have you as extra help to do that supporting work 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the code review, I really didn't see an appreciable benefit to adding them.
I think the rule is, if you add anything outside super(...)
then a test would be required.
But as with anything, it was worth mentioning.
...jmx/src/main/resources/org/springframework/integration/jmx/config/spring-integration-jmx.xsd
Show resolved
Hide resolved
...ration-file/src/main/java/org/springframework/integration/file/FileReadingMessageSource.java
Show resolved
Hide resolved
Looks like a very tedious task. Just be aware, that if you proceed in this manner, you will actually lose the git history of all the moved files. If you want to retain the history you would have to do the moving in one commit and re-introduce the now deprecated classes as new ones in a subsequent commit. Not sure, if it's worth the effort but I just wanted to mention it. |
Thank you for your comment it is very much appreciated. |
That is great idea! Thank you for review!
Yeah... Unfortunately, after removing deprecated classes in the next version we won't have their mentioning anywhere to follow the history. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Do you want me to merge and we start a new PR for the rest of the work, or continue with this PR?
There are just only two modules left. Just state tuned! |
Sounds great! |
Just pushed two commits for JDBC module. |
OK. I think this is all whatever has needed to be done for the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Just a question and a quick doc fix and its ready to go.
/** | ||
* Base package for TCP Support. | ||
*/ | ||
@org.jspecify.annotations.NullMarked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be readded for the deprecated classes?
Fixes: spring-projects#6375 * Move respective classes to the `inbound`, `outbound` or `support` packages to make the whole picture for the API more concise and clean The firs commit is for `stream` module
* Improve `ws.adoc` to avoid `foo`, `bar` etc.
* Move inbound channel adapters into a new `inbound` package * Move outbound channel adapters into a new `outbound` package * Preserve existing classes in the root package as deprecated and as extensions of new classes
* Move inbound channel adapters into a new `inbound` package * Move outbound channel adapters into a new `outbound` package * Preserve existing classes in the root package as deprecated and as extensions of new classes * Some tests refactoring according to modern AssertJ API * Fix typos in `jmx.adoc`
* Move inbound channel adapters into a new `inbound` package * Move outbound channel adapters into a new `outbound` package * Preserve existing classes in the root package as deprecated and as extensions of new classes * Some tests refactoring (affected files) according to modern AssertJ API
* Move inbound channel adapters into a new `inbound` package * Move outbound channel adapters into a new `outbound` package * Move channels into a new `channel` package * Preserve existing classes in the root package as deprecated and as extensions of new classes * Some tests refactoring (affected files) according to modern AssertJ API * Use now a new in SF `SimpleDestinationResolver` for optimization with caching
…ively * Revise affected tests for the modern AssertJ API
and as extensions of moved classes to `inbound` & `outbound` packages
`tcp/outbound`, `udp/inbound` or `udp/outbound` packages, respectively. * Some code clean up for the affected classes and tests improvements according to AssertJ API
to preserve commit history. And mark them as `@Deprecated`
* Bring back `package-info.java` for the `org.springframework.integration.ip.tcp` where we still have deprecated classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Want me to merge?
Fixes: #6375
inbound
,outbound
orsupport
packages to make the whole picture for the API more concise and cleanThe firs commit is for
stream
module