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

Use toString for instance logging #9776

Merged
merged 7 commits into from Sep 11, 2019

Conversation

@milandamen
Copy link
Contributor

commented Aug 20, 2019

Right now, when a qx.core.Object (or subclass) is logged with the qx.log.Logger, a basename and hash is logged. I want to be able to use a overwritten toString to be able to decide what gets logged when the object is logged.

Previous behavior: text = value.basename + "[" + value.$$hash + "]";

New behavior: text = value.toString()
which in qx.core.Object is: text = value.classname + "[" + value.$$hash + "]";

I've tried searching why the logger uses basename instead of classname but I can't think of a good reason it would.

@oetiker
Copy link
Member

left a comment

it would be great if we could skip the whole stringification if logging to the native console as it does a great job with that on its own ...

@derrell
Copy link
Member

left a comment

This will cause a backward-compatibility break in the output. Anyone who had written code that read the logger output will get full paths to class names now when they were previously getting only the basename part of that full path. I'm not opposed to this change -- I think it may have been incorrect to show only the basename part -- but we should be sure we want to consider it a bug and "fix" it, rather than as a BC issue.

@johnspackman

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

I agree that this could cause BC issues, but Im not sure how much of an issue it would be because IMHO it's a reasonable expection that logging output is intended for humans; there's always the possibility of writing a custom Appender class to get direct access to the log entries and the items array.

How about if the code became:

        case "instance":
          text = value.classname + "[" + value.$$hash + "] " + value.toString();
          break;

That would be most likely to preserve any existing parsing and make it more readable. Sometimes the toString() implementations are too anonymous to tell you what the object is anyway.

@cboulanger

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

I assume this is a bug, an accidental use of basename instead of classname. For the sake of BC, though, I would use an environmental variable to enable the behavior intended by this PR by adding something along the lines of

if (qx.core. Environment.get("qx.logObjectToString")) {
   <new behavior>
} else {
   <old behavior>
}

This then can be removed in v7 and the new behavior set as default.

@milandamen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

I would not mind adding an environment variable for this functionality, if you guys agree that that would be the correct option.

@level420

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

@cboulanger do you still have objections against merging this PR?

@level420

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

@derrel do you still have objections against merging this PR?

level420 added 2 commits Sep 11, 2019
@level420

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

merging. thank you @milandamen

@level420 level420 merged commit bb1c1da into qooxdoo:master Sep 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
pullapprove 1 group approved
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.