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

Refactoring: Replace printStackTrace() calls using logger output #7

Closed
wants to merge 1 commit into from

Conversation

Sailsman63
Copy link
Member

This change, originally proposed in PR #3, requires some more thought. While using a logging framework vs. System.out.println() is arguably a more flexible option, we need to consider what we want logging in AOI to look like. As is, this PR will require additional configuration to log to anywhere useful - it currently will not even print to stdout.

If we want to go this route, some further conversion is in order. Some of the standard translators, at least, use System.out.Printerr() and were missed in the original conversion. @peastman, your input would be appreciated.

@Sailsman63 Sailsman63 mentioned this pull request Sep 22, 2016
@peastman
Copy link
Contributor

I think you summarized the situation well. The advantage of using a logging framework, rather than just printing to stdout, is that it's more flexible. But that's only an advantage if you actually use that flexibility.

Is there demand for improved logging features? If so, we should discuss what those are, then make this change as part of implementing them. If not, then let's not fix what isn't broken.

@makiam
Copy link
Contributor

makiam commented Sep 23, 2016

Main idea to see and differentiate logging data importance.
Easy to configure more advanced logging options ex. log to file...

@makiam
Copy link
Contributor

makiam commented Sep 24, 2016

I create commit to fix formatting: Format

@Sailsman63
Copy link
Member Author

Sailsman63 commented Sep 24, 2016

@makiam, There's been enough confusion with the revision history that I'm finding it difficult to apply your latest. Your fork history is now messed up enough that you will find it difficult to do pull requests going forward. I'm sorry, some of that was my fault. I'm going to suggest a set of steps to clean it up, starting in your master branch:

  • git reset --hard <upstream> 'upstream' referring to whichever branch you are pulling down the official AOI repo to. This will clear out all of the now outdated commits, etc.
  • git push -f to make those cleanups appear on your github account.

So far, so good? The next few are for any time you would like to set up some new contributions to any github project:

  • git branch <name-of-new-contribution> You don't actually want to commit your contributions directly in master - if you do, you'll end up with more cleanup work
  • git checkout <name-of-new-contribution> Move over to your new branch. This is where you want to do your work.
  • git push origin <name-of-new-contribution> puts the changes up under your account in a separate branch. Start your pull request from this branch. Use a different branch for changes related to a different feature or bug.

@Sailsman63
Copy link
Member Author

We also need to figure out what use, if any, we would make of a more flexible logging system. When trying to debug user issues, we usually want just about everything possible. I'm not seeing a reason to have multiple logging levels at this point.

How would a user configure logging to file? Many of our users are not very technical, so we end up posting a command-line start option for them that redirect stdout and stderr to a text file.

@makiam
Copy link
Contributor

makiam commented Oct 3, 2016

I think we may drop this PR at this moment,,,

@Sailsman63
Copy link
Member Author

This branch can't really be merged anymore. The question of improving the logging story is continued in #50

@Sailsman63 Sailsman63 closed this Aug 18, 2017
@Sailsman63 Sailsman63 deleted the makiam-logger branch November 17, 2021 07:42
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.

None yet

3 participants