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

Consider moving jOOQ configuration stuff into a jooq element #12

Closed
lukaseder opened this issue Jun 2, 2023 · 2 comments · Fixed by #13
Closed

Consider moving jOOQ configuration stuff into a jooq element #12

lukaseder opened this issue Jun 2, 2023 · 2 comments · Fixed by #13

Comments

@lukaseder
Copy link

The plugin configuration looks like this:

<configuration>
        <!-- Plugin configuration -->
	<database>
	</database>

        <!-- delegation to flyway configuration -->
	<flyway>
	</flyway>

        <!-- delegation to liquibase configuration -->
	<liquibase>
	</liquibase>

        <!-- jooq configuration ? -->
	<generator>
		<database>
			<includes>.*</includes>
			<excludes>flyway_schema_history</excludes>
			<inputSchema>public</inputSchema>
		</database>
		<target>
			<packageName>org.jooq.codegen.maven.example</packageName>
			<directory>target/generated-sources/jooq</directory>
		</target>
	</generator>
</configuration>

I think it would be more clear to put jOOQ stuff inside a jOOQ element. I'm aware that this would produce an apparently unnecessary additional level of nesting, but:

  • Even in jOOQ, <generator/> isn't a top level element. While <jdbc/> isn't necessary in most cases, it may be in some (e.g. to pass additional properties to the JDBC connection)
  • There are elements like <skip/>, <logging/>, <onError/>, <onUnused/>, <configurationFile/>, and others that might be added in the future, which you won't support out of the box unless you nest jOOQ's configuration in a <jooq/> element. Note, you could add support for these in your own top level, but then that would be confusing as it wouldn't be clear if e.g. <skip/> belongs to jOOQ, or to you.
  • Who knows if you're going to add more additional elements in the future, e.g. to clean up / post-process generated code, etc. Those elements will also possibly conflict with jOOQ stuff.
@sivaprasadreddy
Copy link
Contributor

sivaprasadreddy commented Jun 2, 2023

Hi @lukaseder
Thanks for the suggestions. As you suggested, moving jOOQ related config inside <jooq>...</jooq> would be less confusing for people.

@romchellis would you like to work on these changes? If we can incorporate these changes by next week we can release 0.0.2 along with these changes.

@romchellis
Copy link
Contributor

Hi @sivaprasadreddy , @lukaseder .
I like suggested approach.
Actually I'am already looking on possible solutions.Not sure that it will be completed by monday, possibly by the end of next week.

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 a pull request may close this issue.

3 participants