-
Notifications
You must be signed in to change notification settings - Fork 33
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
Deprecate disunified repos #1
Comments
Thanks for the work; I'm trying to think if it is worth at all to have these projects separated at this point. They share the same package naming so it's fairly easy to merge all of them into a super project. What do you think? |
Just to be sure we're on the same page: you're suggesting taking an If it were up to me, I wouldn't go that far. I'd reduce it to two modules: However, didn't you tell me before that you originally split them up into On Tue, Jul 26, 2016 at 11:54 AM, Jinho D. Choi [email protected]
|
Yes, that was the concern, but now we have modulated the code much better, I think it should be ok (NLP4J, previously called ClearNLP, even before called ClearParser, has evolved a lot since 2011 :) I'd take the suggestion of separating CLI to the pure API. If you'd like to do this merge, I can spend some time later today to review the code and finalize the unification. Thanks! |
I'll put the stream business aside for now and do the unification today. On Tue, Jul 26, 2016 at 12:16 PM, Jinho D. Choi [email protected]
|
I've put up a PR for this. I think I've accounted for everything. It's a bit disorienting. |
I hope you won't be too annoyed that I added some very small and local changes that rescue the useful part of the big file/stream PR. It's completely compatible. If you don't like it, let me know, and I can move those commits to another PR. I was sitting on the wrong branch, and I need to go do an errand with my wife, so I decided to just push this up to you and see if it was OK. |
I just merged to PR. I'm going to take a look and see if there are changes needed now. Will get back to you soon. |
I updated the pom.xml of submodules, removing the versions for the submodules. If I remember right, this would adapt the parent's version, but please let me know if this should be back. I also made a small change due to the upgrade of fastutils. Thanks. |
I'll have a look. On Wed, Jul 27, 2016 at 10:21 AM, Jinho D. Choi [email protected]
|
I just moved the tokenization package to edu/emory/mathcs/nlp/component/tokenizer/ . I'll be testing each module this afternoon. |
Your POM change was correct. The only version needed in a submodule is the On Wed, Jul 27, 2016 at 10:28 AM, Benson Margulies [email protected]
|
There will be a small celebration up here once we have a release from this On Wed, Jul 27, 2016 at 10:28 AM, Jinho D. Choi [email protected]
|
Haha yes, also, how about calling the "commands" project as "cmd" or "cli"? I should check what people usually call this kind of project. |
I don't have a gut sense of the relative popularity of these options. I do On Wed, Jul 27, 2016 at 10:59 AM, Jinho D. Choi [email protected]
|
I like 'cli' as well, let me see if i can make this change. |
Another thing that we could do here at some point is to add the On Wed, Jul 27, 2016 at 11:06 AM, Benson Margulies [email protected]
|
That's a great idea; i've been doing that manually but maven plugin should help. |
@jdchoi77 Here is the unified repo. I will proceed from here in PR's for other work. The readme might want some more work to explain the structure, let me know if you want me to do it.
The text was updated successfully, but these errors were encountered: