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

MODE-1462 - Updated logging: #393

Merged
merged 2 commits into from
May 29, 2012
Merged

MODE-1462 - Updated logging: #393

merged 2 commits into from
May 29, 2012

Conversation

hchiorean
Copy link
Member

  • moved all logging classes to common.logging instead of common.util
  • added logging factory & logger for pure Log4j support
  • changed logging dependencies scope from "compile" to "provided"
  • added a logger abstraction in modeshape-jcr-api and an implementation in modeshape-jcr which should be used for extensions
  • updated Sequencer and TextExtractor base classes so that upon creation, they are provided with the extension logger

- moved all logging classes to common.logging instead of common.util
- added logging factory  & logger for pure Log4j support
- changed logging dependencies scope from <compile> to <provided>
- added a logger abstraction in modeshape-jcr-api and an implementation in modeshape-jcr which should be used for extensions
- updated Sequencer and TextExtractor base classes so that upon creation, they are provided with the extension logger
@@ -62,7 +54,7 @@
* @return the current locale used for logging, or null if the system locale is used
* @see #setLoggingLocale(Locale)
*/
public static Locale getLoggingLocale() {
public Locale getLoggingLocale() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about making this method final? It's now called inside each of the various log methods (which we want to be as fast as possible), so adding 'final' makes it more likely (all things considered) to be inlined.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good, will update

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I shouldn't have made the method non-static, because it hides the intent - all the loggers share the same locale - so I'll make it static, so final won't make sense really

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good point.

@rhauch
Copy link
Contributor

rhauch commented May 29, 2012

One more question. Does this mean that many/most/all of our sequencer implementations no longer need to depend on modeshape-common?

@hchiorean
Copy link
Member Author

They still do (most of them) mainly because of the annotations. There's also a few (ddl most notably) which uses also the .text and .component subpackages from common.

@rhauch rhauch merged commit 9962a8b into ModeShape:master May 29, 2012
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