-
Notifications
You must be signed in to change notification settings - Fork 41
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
[DOXIA-534] Migrate logging to Sl4j #5
Conversation
@@ -78,7 +72,7 @@ under the License. | |||
<dependency> | |||
<groupId>commons-collections</groupId> | |||
<artifactId>commons-collections</artifactId> | |||
<version>3.2.1</version> | |||
<version>3.2.2</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a seperate issue if not necesary for this PR.
pom.xml
Outdated
</dependency> | ||
</dependencies> | ||
</dependencyManagement> | ||
<dependencies> | ||
<dependency> | ||
<groupId>junit</groupId> | ||
<artifactId>junit</artifactId> | ||
<version>4.11</version> | ||
<version>4.12</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as commons-collections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will be diffret pull - only with dependences
@@ -1322,12 +1322,12 @@ private SinkEventAttributeSet getGraphicsAttributes( String logo ) | |||
} | |||
catch ( IOException e ) | |||
{ | |||
getLog().debug( e ); | |||
getLog().debug( e.getMessage() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave it as e
, SFL4J and backends will do the rest for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no constructor with Exception, i will change to e.getMessage(), e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slachiewicz @michael-o Proper code here would be getLog().debug("", e)
. That assumes you want the full stack trace and not just the message.
@hboutemy Looks good to me. Tests pending... |
I'm not in favor of removing doxia-logging: deprecating and modifying it to use slf4j would be more compatible |
Why not deprecate in 1.8 and remove with 2.0? |
+1 |
@slachiewicz Can you spawn another ticket and create a new PR? We will leave this one open. |
There is almost no usage of logging in Doxia, so any extention needs only to remove one call. |
New version with deprecation in #8 |
I mantain the asciidoctor extension and I am fine with removing logging-api. In fact we have problems sharing code between the our normal Mojo and the doxia parser because one uses "maven.plugin.logging.Log" and the other "maven.doxia.logging.Log". Moving doxia to Sl4j won't fix it 100%, but will be one step closer. |
* Bumped versions * Migrated site module to slf4j since apache/maven-doxia#5 * Reorder constructor args in AsciidoctorDoxiaParserModule
* Bumped versions * Migrated site module to slf4j since apache/maven-doxia#5 * Reorder constructor args in AsciidoctorDoxiaParserModule
* Bumped versions * Migrated site module to slf4j since apache/maven-doxia#5 * Reorder constructor args in AsciidoctorDoxiaParserModule
* Bumped versions * Migrated site module to slf4j since apache/maven-doxia#5 * Reorder constructor args in AsciidoctorDoxiaParserModule
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule Fixes asciidoctor#578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule Fixes asciidoctor#578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule Fixes asciidoctor#578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes asciidoctor#578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes #578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes #578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes #578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes asciidoctor#578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes asciidoctor#578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes asciidoctor#578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes asciidoctor#578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes asciidoctor#578
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes asciidoctor#578 Bump doxia-core to v2.0.0-M3 (asciidoctor#590) * Bump maven-site-plugin in IT to v4.0.0-M3 * Add skin to IT (mandatory since v4.0.0-M2) Bump maven-site and doxia to latests * maven-site-plugin v4.0.0-M8 * doxia & fluido skin 2.0.0-M6 * Sections and tables updated Bump maven-site to M13, Doxia to M8
* Bump Doxia to 2.0.0-M1 * Leave note in docs for release * Use SLF4J in site module (apache/maven-doxia#5) * Reorder constructor args in AsciidoctorDoxiaParserModule * Update IT test site.xml Fixes asciidoctor#578 Bump doxia-core to v2.0.0-M3 (asciidoctor#590) * Bump maven-site-plugin in IT to v4.0.0-M3 * Add skin to IT (mandatory since v4.0.0-M2) Bump maven-site and doxia to latests * maven-site-plugin v4.0.0-M8 * doxia & fluido skin 2.0.0-M6 * Sections and tables updated Bump maven-site to M13, Doxia to M8
* Use SLF4J in site module (apache/maven-doxia#5) * Unify Doxia versions in parent pom * Use same Doxia version for doxia-core and doxia-site-renderer * Use javax.inject.Named instead of custom Plexus * Add skin to IT (mandatory since v4.0.0-M2) * Bump maven-site-plugin, doxia, Fluido skins
* Use SLF4J in site module (apache/maven-doxia#5) * Unify Doxia versions in parent pom * Use same Doxia version for doxia-core and doxia-site-renderer * Use javax.inject.Named instead of custom Plexus * Add skin to IT (mandatory since v4.0.0-M2) * Bump maven-site-plugin, doxia, Fluido skins
* Use SLF4J in site module (apache/maven-doxia#5) * Unify Doxia versions in parent pom * Use same Doxia version for doxia-core and doxia-site-renderer * Use javax.inject.Named instead of custom Plexus * Add skin to IT (mandatory since v4.0.0-M2) * Bump maven-site-plugin, doxia, Fluido skins
Please check.
Current build fails with cirr error - due to interfaces change.