Skip to content

migrate to gradle#5

Open
JonasFranke wants to merge 15 commits intoClym-Dev-Team:masterfrom
JonasFranke:dev/gradle
Open

migrate to gradle#5
JonasFranke wants to merge 15 commits intoClym-Dev-Team:masterfrom
JonasFranke:dev/gradle

Conversation

@JonasFranke
Copy link
Copy Markdown
Contributor

Gradle > Maven

Copy link
Copy Markdown

@leonbcode leonbcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .gradle directory should be excluded from version control, as it contains files and information that are specific to the Gradle build process and don't need to be tracked by Git.

Copy link
Copy Markdown

@leonbcode leonbcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using the Spring Boot Gradle Plugin as the build plugin, as Spring Boot is already being utilized in the currently used maven build process.

@leonbcode
Copy link
Copy Markdown

I've done some testing and it seems that your changes don't seem to work on my machine at all, either it's a fault on my end, or there are some major flaws in your proposed changes.
I had to modify quiet a lot of things to get the Gradle migration working, which made me wonder, have you tested your code at all? (and if not, I think you shouldn't open any pull requests with untested code in the future)

Now the changes I had to make to get it working:

  • move src/main/ to src/main/java/some-package-name
  • refactor imports and package statements accordingly
  • change the group in build.gradle to the chosen package name
  • move src/resources/ to src/main/resources
  • remove following plugins from build.gradle: java-library, maven-publish, com.github.johnrengelman.shadow as well as any configuration regarding these plugins
  • some minor changes (I don't quite remember)

Hence the migration from Maven to Gradle is a major change regarding the project structure, I'd recommend to address this change in a future meeting.

@JonasFranke
Copy link
Copy Markdown
Contributor Author

which gradle task did you (want to) run?

@JonasFranke
Copy link
Copy Markdown
Contributor Author

and why did you have to remove the plugins?

@JonasFranke JonasFranke changed the title change maven to gradle migrate to gradle Feb 10, 2023
@leonbcode
Copy link
Copy Markdown

and why did you have to remove the plugins?

The Spring Boot Gradle Plugin has packaging and dependency management capabilities on it's own and can work as a replacement for the other plugins. (https://docs.spring.io/spring-boot/docs/current/gradle-plugin/reference/htmlsingle/#introduction)

@leonbcode
Copy link
Copy Markdown

which gradle task did you (want to) run?

bootRun and bootJar, now with the changes in 04f2447 it seems to work.

Comment thread src/main/resources/logback.xml Outdated
<logger name="main.main.system.eventSystem" level="INFO" />
<logger name="main.main.system.commandSystem" level="INFO" />
<logger name="main.main.system.commandSystem.CommandProcessor.Chat" level="DEBUG"/>
<logger name="main.main.system.ASCIIProgressbar" level="INFO" />
Copy link
Copy Markdown

@leonbcode leonbcode Feb 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here should be main. instead of main.main.

@leonbcode
Copy link
Copy Markdown

Generally I would not recommend a package with the name main., as this isn't very meaningful. Rather I suggest using a package name, which follows the Package Naming Conventions.

@Orciument
Copy link
Copy Markdown
Member

We can do that, I would only suggest keeping the nesting to a minimum because I am not a fan of deep nesting

Copy link
Copy Markdown

@leonbcode leonbcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we wait with the merge until, we as a team have decided on a project name, as the package name should be related to the project name.

private static Set<Method> scanForSubscriber() {
// Reflections reflections = new Reflections("main.modules", Scanners.MethodsAnnotated);
Reflections reflections = new Reflections(new ConfigurationBuilder().forPackages("main.modules").addScanners(Scanners.MethodsAnnotated));
// Reflections reflections = new Reflections("main.main.modules", Scanners.MethodsAnnotated);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use main.modules instead of main.main.modules


private static Set<Method> scanForHooks() {
Reflections reflections = new Reflections(new ConfigurationBuilder().forPackages("main.modules").addScanners(Scanners.MethodsAnnotated));
Reflections reflections = new Reflections(new ConfigurationBuilder().forPackages("main.main.modules").addScanners(Scanners.MethodsAnnotated));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use main.modules instead of main.main.modules

@@ -1,4 +1,4 @@
package main.inputs.Twitch4J;
package main.Twitch4J;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Twitch4J should be a submodule of inputs, as there will be added more inputs, for example TipeeeStream.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, please keep the original folder structure intact.
All Inputs should be in the Inputs directory and implement the TwitchBotInput Interface.

More generally, if someone has questions about implementing something, like me. Then I know what I should include in the wiki articles (that I'm going to write when I have time for that)

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.

3 participants