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

Extend logging to whole project #772

Open
Ryuno-Ki opened this issue Oct 31, 2018 · 18 comments

Comments

Projects
None yet
5 participants
@Ryuno-Ki
Copy link
Contributor

commented Oct 31, 2018

After #763 was merged, let's extend it to the rest of the code base, so we can get rid of console.log statements.

@Ryuno-Ki

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

@jkasun

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

Can i fix this issue @turt2live

@Ryuno-Ki

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Fine from my site :-)
(Forgot about this issue ^^")

@jryans

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@jkasun Feel free to work on this if you'd like to!

@jkasun

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

Okay im on it

@jkasun

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Some spec files also have console.log statements. Do we change that also? @Ryuno-Ki

@jryans

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Might as well include spec files as well, if possible. If it's hard to do for some reason, then you could leave them out for now.

@Ryuno-Ki

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@jkasun Do you have something for me to look at?
Hard to tell off-hand.
As rule of thumb I'd say, yes. Because console.log is always written to STDOUT (thus spamming a log file if used).

@jkasun

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@Ryuno-Ki I change all the console.log to logger method in spec file and source files. But I kept example files and debuglog as it is for now, should we change them also? Here is a file from example and sample code of debug log.

Here is files that is using console.log, but I guess it might be okay since those are examples,
https://github.com/matrix-org/matrix-js-sdk/blob/develop/examples/browser/browserTest.js
https://github.com/matrix-org/matrix-js-sdk/blob/develop/examples/node/app.js

Here is a sample of debug log and should we change them to logger also,

Sample 01

const debuglog = DEBUG ? console.log.bind(console) : function() {};

Sample 02

function debuglog(...params) {
    if (!DEBUG) {
        return;
    }
    console.log(...params);
}

for now I just kept them as they were is it okay to change that to the logger also?

Those examples are from
https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/store/session/webstorage.js
https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/timeline-window.js

@jkasun

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Hi @Ryuno-Ki, can you help me with this?

@jryans

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I would suggest asking in #riot-dev:matrix.org if you need help. @Ryuno-Ki does not work on Riot full time, so I think you may get a faster answer in a room where many people can see you question.

@jkasun

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@jryans I already did. No one is replying

@jryans

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Ah okay. Unfortunately, the core team is quite busy at the moment, so that could explain it.

@jryans

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I tried to scan your question above... I'd say leave the examples alone. For debuglog, probably it does make sense to change these ones so that everything in actual SDK code (not examples, etc.) uses the same logging path.

@Ryuno-Ki

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

#772 (comment)

Not full-time?
Not at all...
Currently I've got other prios.
Still need to create a new password due to that security issue earlier this year + pw reset. Let me look at this PR.

@Ryuno-Ki

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

const DEBUG = false; // set true to enable console logging.

Can we try to use an environment variable?
`process && process.env && process.env ? process.env.NODE_ENV !== "production" || false".

If it runs in the process the value (debug on/off) might be stored in localStorage.
This way the developer could switch it without a rebuild.

debuglog("TimelineWindow: increased cap by " + count +

Is it possible to use template strings here?

debuglog("paginate: switched to new neighbour");

You aren't adding the TimelineWindow prefix here. As a pattern it might be possible to read the callee from this.name (empty in anonymous functions) and use that in the bind of debuglog so to not repeat it over and over. At the very least you could insert the class name there.

console.log("data %s [...]", JSON.stringify(data).substring(0, 100));

I believe this one isn't needed to be logged by default. The other console.log could be helpful. No strong opinions here.

var myAccessToken = "QGV4YW1wbGU6bG9jYWxob3N0.qPEvLuYfNBjxikiCjP";

@Kegsay I hope you recreated a new token by now...
Can this be read with dotenv from a .env as environment variable?

console.log.apply(console.log, arguments);

Maybe allow a loglevel as well. Then console[loglevel].apply it.

@Kegsay

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@Ryuno-Ki : This was an example access token, not a real one :) thanks though.

@jkasun

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@Ryuno-Ki, @jryans Thank you. I will obviously keep the example files as it is and change the debug log as required. 😄

@jkasun jkasun referenced a pull request that will close this issue May 19, 2019

Open

Extend loglevel logging for the whole project #924

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.