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

fixed exception logging #212

Closed

Conversation

sergiorussia
Copy link
Contributor

@samskivert, hi. found small issue in logger. w/o fix:

...
2019/05/05 20:24:49:266 INFO Application requires update.
2019/05/05 20:29:53:149 WARNING Download failed [rsrc=patch201904141803.dat, java.io.FileNotFoundException: http://app/files/201905051855/patch201904141803.dat=<toString() failure: java.lang.ArrayIndexOutOfBoundsException: 3>]
java.io.FileNotFoundException: http://app/files/201905051855/patch201904141803.dat
...

wrote a test which initially failed:

Expected :Download failed [rsrc=Resource]
Actual   :Download failed [rsrc=Resource, java.io.FileNotFoundException: patch.dat=<toString() failure: java.lang.ArrayIndexOutOfBoundsException: 3>]

i got a question though. why not change Log.format method to accept Map<String, Object> instead of Object[]? Map is verbose to write of course but better reflects method requirements and less error-prone.

@samskivert
Copy link
Member

I ended up fixing it differently in 339faa8. Getdown used to use another library which did this log formatting, but I wanted to remove the dependency on that library so I rewrote the log handling stuff and introduced this error.

As for why not to use a map, the whole point of the simple Log API is to make it easy to log a message and simple key value pairs. As in this:

log.info("Some message", "key", value, "key2", value2);

Is more pleasant to write than:

log.info("Some message [key=" + value + ", key2=" + value + "]");

If you had to go to all the trouble to create a map to pass key/values, you might as well just make the string yourself because that would be simpler.

Anyhow, it's an ancient code base, written against an ancient API, in an ancient language. I'm not really looking to make any sweeping changes.

@samskivert samskivert closed this May 9, 2019
@samskivert
Copy link
Member

But thanks for finding and sending a PR to fix this bug!

@sergiorussia sergiorussia deleted the bugfix/log-with-throwable branch May 9, 2019 19:21
@sergiorussia
Copy link
Contributor Author

@samskivert you're welcome. may i ask you to release a new Getdown build to mvn? :)

@samskivert
Copy link
Member

There's one PR in discussion that I'd like to let potentially get resolved in the next day or two, but I'll make a note to do a release Monday if that PR doesn't look like it's going to get sorted out.

@sergiorussia
Copy link
Contributor Author

sergiorussia commented May 14, 2019

@samskivert any chances to get a new release today? 👀 😊

@samskivert
Copy link
Member

Shipped it! :)

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

2 participants