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

Rename the ILogger interface to Logger #1675

Open
kittaakos opened this issue Apr 10, 2018 · 9 comments
Open

Rename the ILogger interface to Logger #1675

kittaakos opened this issue Apr 10, 2018 · 9 comments
Labels
help wanted issues meant to be picked up, require help quality issues related to code and application quality

Comments

@kittaakos
Copy link
Contributor

Rename the ILogger interface to Logger, so that it is aligned with our Coding Guidelines. Nothing more.

@kittaakos kittaakos added help wanted issues meant to be picked up, require help quality issues related to code and application quality labels Apr 10, 2018
@lmcbout
Copy link
Contributor

lmcbout commented Apr 10, 2018

According to issue #624, should it be LoggerImpl ?

@kittaakos
Copy link
Contributor Author

You mean, the implementation; yes. LoggerImpl is good. We do not see that too much often anyway.

@lmcbout
Copy link
Contributor

lmcbout commented Apr 10, 2018

I agree

@vince-fugnitto
Copy link
Member

I'm not sure if renaming ILogger to Logger would be appropriate at the moment since it will most likely break a lot of external extensions still referring to the original name. Perhaps in the future when we have our first main release (ex: 1.0.0), we can take time to clean up the code, and any extensions would have to align with our new codebase.

@akosyakov
Copy link
Member

as public API using console.log is preferable

At the beginning, we thought that Logger was a good idea, but after realizing that 3rd party libraries could not use it, decided to allow console.log and hook it with Theia.

@tsmaeder
Copy link
Contributor

@akosyakov that leaves the question how to filter log output, though. What's the story there when using console.trace, etc?
In development, we often want to turn on debug and trace info. Adding console.log statements each time does not scale.

@akosyakov
Copy link
Member

akosyakov commented Oct 29, 2019

In development, we often want to turn on debug and trace info. Adding console.log statements each time does not scale.

You can call console.trace and console.debug, they are delegated respectively to Logger.trace and Logger.debug. Logging level can be configured by passing --log-level cli option to theia start.

@tsmaeder
Copy link
Contributor

tsmaeder commented Oct 29, 2019

So console.log is redirected to rootLogger.log, which again goes to the console? Anyway...still not reason not to use ILogger, it allows to turn on/off logging per named log, not globally.
We have the same situation in Java, where different components will use different logging mechanisms, but that's not a reason to use printf directly.

@akosyakov
Copy link
Member

So console.log is redirected to rootLogger.log, which again goes to the console?

If someone logs in the browser content, the root logger ensures that all logs end up on the backend, not only in the browser console. By default, yes, they are forwarded to the backend console. End products can overwrite it and integrate with own logging infrastructure. For example, Gitpod wraps logs in JSON structures and forward them to Google Stackdriver for analysis.

Anyway...still not reason not to use ILogger, it allows to turn on/off logging per named log, not globally.

One can do it when necessary. Theia coding guidelines are not something to copy for end products. Also within Theia for child logging using Logger is fine. I will update the coding guidelines to prefer console only for the root logging.

We have the same situation in Java, where different components will use different logging mechanisms, but that's not a reason to use printf directly.

It is not the same. In Java printf using stdout not the logging framework. In Theia, when someone calls console our Logger is used, not the native console. If someone likes boilerplate they can inject the root logger each time to call log on it. Although console.log does exactly the same in any contexts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted issues meant to be picked up, require help quality issues related to code and application quality
Projects
None yet
Development

No branches or pull requests

5 participants