-
Notifications
You must be signed in to change notification settings - Fork 18
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
Dendency Cycles removed #40
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # jsettlers.buildingcreator/build.gradle # jsettlers.logic/src/main/java/jsettlers/ai/army/RegroupArmyModule.java # jsettlers.main.swing/build.gradle # jsettlers.tools/build.gradle
* Resolved the dependencies and created a dependency hierarchy. The hierarchy is shown in ProjectStructure.uxf. There are no longer cycles in the dependency tree
These are a lot of changes and I don't think they belong together:
The classes in testutils actually did serve a purposes. For once their only use is testing this is why testutils is only included as a test dependency. This mean that the classes in testutils don't belong in src/main/ but rather in src/test. This is especially true for the replay-it stuff. I also don't think its completely possible to remove the testutils module because the AutoReplaySettings are used by the jsettlers.tools module to regenerate new validation savegames but AutoReplaySettings is also needed by the test cases but it definitly is just a test helper and it shouldn't be part of the actual release.
I agree that GSON should probably just be a global dependency. |
You are right, not all of this belongs together. I am going remove the refactoring in NetworkTimer. (It wasn't meant to be in this PR. I made a mistake in my own branch management. Those changes were more like a warm up, and I wasn't finished with them) For the imports, I am going to use the formatter mentioned in the guidelines and remove all the unnecessary import changes. Makes this a little clearer. I understand that testUtils had its place. I think it makes sense to extract the tests from the release Modules. But the Logic shouldn't be dependend on anything test related. I think it is necessary to refactor that part, so that the logic has all the util functions it needs. And that the tests can access them (if it really needs those). For now i thought it best to have the tests in the logic, so they can still be run properly. But maybe they can at least be put into src/test. I'll give that a shot. For the eclipse part. That was on my side. Is it a problem to have the plugin in the gradle files? (I tried to add it to the main build file, so that all subprojects would inherit it, but it didn't seem to setup the project correctly). I would prefer to keep developing with eclipse rather than switching to IntelliJ. I know that android is not supported anymore. I actually think it might be better to split the android part somehow from the rest. (No idea how much work that would require). But personally it was a real knock back that i couldn't get the project to run without android. It would be nice if i could decide if i want to have it run on desktop only, or if i want to include the android part. |
This is a bad idea. If personally don't use this specific ruleset and I don't really care about what formatter you use as long as the code is still readable and indented with tabs.
All android specific code is in go.graphics.android and jsettlers.main.android.
I agree. If I think about it it might be enough to move anything that belongs to the tests of jsettlers.logic to its src/test/java directory except for the AutoReplayIt class which is needed by both jsettlers.logic and jsettlers.tools and then remove all dependencies of jsettlers.testutils to break the cycle (unless there is another one of course) |
Okay, then i will leave the import changes be. Concerning android: When I first tried to build the project with gradle I got exceptions that it was missing android dependencies. But now that I think of it, I might still have had an older Android sdk installed. Maybe that was the problem. At least now it builds just fine. I also looked into the dependencies of the android modules. I'll make sure they still work. Since I got my SDK running I can also use it. |
no, please remove all import changes that aren't necessary but don't use any specific code formatting tools because it will just add more unnecessary changes |
Most of the changes are imports. Partially because of the change of the modules, and partially because of the formatter.