-
Notifications
You must be signed in to change notification settings - Fork 703
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 our logger instead of console.{log,error} for LDAP logs #552
Conversation
Actually we left those as |
I couldn't find discussion about this on GitHub, maybe it was IRC. Should we get rid of https://github.com/thelounge/lounge/pull/552/files#diff-c634a78f476381115a1c873dc965554dR10 then? Also, this is not the important part of the PR lol, what about the other changes (related to recent LDAP support, #477)? |
486418b
to
14782a5
Compare
Ah yes, remove that and the other part is definitely 👍 especially the throw. Confused the two PRs for a moment 😅 |
@maxpoulin64, removed the changes in (Maybe wait for @thisisdarshan and/or @lindskogen to comment on this before merging) |
Actually I -1'd to prevent a merge because I can't add labels on mobile 😶 |
I'm not too fond of crashing the app but that's definitely one option (the one currently, which would change by getting rid of the A third option is to assume mode is private when Thoughts? |
Okay I'm just bad at communicating tonight. Meant, the other stuff is good changes, including the removal of the throw 😅 |
Use our logger instead of console.{log,error} for LDAP logs
Ping @thisisdarshan and @lindskogen, could you let us know if the LDAP part looks fine to you?
Note that when running a public instance and LDAP, the LDAP part is now ignored and a warning is shown instead of crashing the app. @maxpoulin64, does that seem sensible to you?