-
Notifications
You must be signed in to change notification settings - Fork 181
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
Issue #1753 remove non CE features #1759
Issue #1753 remove non CE features #1759
Conversation
Whoah, that's a LOOOOT of code removed. I'll start digging :) (I should probably check the branch out for the code review, so I can see the tree properly. I guess it looks more crazy in list form, because whole modules has been nuked, and those I can essentially ignore completely as long as I agree with the module removal itself) |
Just curious: Any reason for the fork instead of a branch (same with the other PR), just habit? :) |
With OSS projects I like to use forks so as to not needlessly exercise my administrative privileges, but rather go through the "contributor" experience ;-) |
Hmmmm... My internal pedant wants to point out that it's not administrative privileges, but makes sense :) |
Do we really want to get rid of the ExtensionSwap? I realize that it'll need to be reworked with an internal UI (e.g. using something like a simple GitHub-pages hosted JSON that links to the extensions, which could in most cases simply be release files in GitHub), but it seems like a bit of a shame to completely lose it. OTOH, if anyone does that work, s/he'd probably have to start all over anyway, and we still have the base extension loading, so it's probably all good. BTW, you left in the "Browse the ExtensionSwap" menu item in options/Extensions, was that on purpose? |
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.
Two minor issues, otherwise LGTM.
@@ -32,7 +32,6 @@ | |||
import org.apache.commons.lang.ArrayUtils; | |||
import org.apache.metamodel.util.Action; | |||
import org.datacleaner.actions.DownloadFilesActionListener; | |||
import org.datacleaner.actions.PublishResultToMonitorActionListener; |
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.
There's a reference to this class in the JavaDoc below
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.
Fixed
* needed. | ||
*/ | ||
@Deprecated | ||
public static <K, V> ConcurrentMap<K, V> createCacheMap() { |
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.
ConcurrentMap is now an unused import
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.
Fixed
LGTM! |
Fixes #1753
This is a rather aggressive cleanup in our codebase to remove non-CE relevant code. Here's a listing of stuff I removed (got my hands proper dirty this time so I hope I remember all):