Skip to content
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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Blackwolf155
Copy link

Most of the changes are imports. Partially because of the change of the modules, and partially because of the formatter.

# 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
@paulwedeck
Copy link
Owner

These are a lot of changes and I don't think they belong together:

  • please split all the import refactoring into a separate PR (if you even care about them at all)
  • You did some changes that refactored actual code like in NetworkTimer. I partially agree with the changes but this should be its own PR

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.

  • Is there any particular reason why you added eclipse to every module?
    Google says that this helps importing this project into eclipse but I don't see any particular reason why we would continue to support eclipse considering that it no longer has any Android support. Am I missing something?

I agree that GSON should probably just be a global dependency.
I also don't actually know why we have a junit.jar and hamcrest-core.jar in the repo but I agree that it probably should be removed.

@Blackwolf155
Copy link
Author

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.

@paulwedeck
Copy link
Owner

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.

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.

I actually think it might be better to split the android part somehow from the rest. (No idea how much work that would require)

All android specific code is in go.graphics.android and jsettlers.main.android.
If you don't have the android sdk the android modules should just be ignored unless something broke.
Please note that this also means that parts of the android code might no longer compile without you noticing so please take care that your changes don't affect the android modules.

But the Logic shouldn't be dependend on anything test related

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)

@Blackwolf155
Copy link
Author

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.

@paulwedeck
Copy link
Owner

Okay, then i will leave the import changes be.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants