-
Notifications
You must be signed in to change notification settings - Fork 22
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
Added a Settings Command. #72
Conversation
First of all, you are using the same code for each setting to print in the desired color the value, make that a method. Second of all, you could make a method inside the config class and return that value. And third, there is no need to color anything, just return config::ToString() Anyways that is my personal feedback, wait for Zabszk to see what he says. |
Hi, First of all conversion of the config to string already exists here: Lines 234 to 280 in 593f190
Secondly LocalAdmin is a multithreaded application. Printing output of this command line by line (using separate Write/WriteLine commands) will sooner or later cause an issue where two different Write/WriteLines run concurrently on different threads. That will result in output from this command being mixed with a different text (printed on a different thread). You additionally set foreground color, which will make the situation even worse. And LocalAdmin-V2/Core/LocalAdmin.cs Lines 20 to 29 in 593f190
|
Honestly, I just wanted to get it done. And I had it in a method but is would make the code a bit longer than already. But yes, I'm working on the method part. Secondly, I understand the mixing issue and I'm trying to figure out the best way to implement it without that issue. And I am trying to find a better solution for it. |
That might be the better solution. I'll try to change it to that now. |
It should be an easy command smt like this: internal override void Execute(string[] arguments) => ConsoleUtil.WriteLine(Configuration.ToString()); Again, personal feedback. Also about the command name I would maybe change it to smt more LA related so it doesnt break any type of plugins that may be using that. But zabszk may have their opinion about that. |
Yes. Something like this should be enough.
While plugins don't interact with LA, having a LA command named |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally keep in mind that "help" command isn't automatically generated. You need to add the command there as well.
Yeah, that was what I was afraid of, server console commands. With breaking I meant being overrided. |
It still gives the same idea. But I get where your trying to point. |
Co-authored-by: Łukasz Jurczyk <zabszk@protonmail.ch>
Based on suggestion by zabszk. Co-authored-by: Łukasz Jurczyk <zabszk@protonmail.ch>
The code looks good. Can you update the command description in the "help" command? |
Yes, I can do that rn. |
Feel free to make suggested changes. |
Added a settings command which will print out the configuration information with colors.
Green - True
Red - False
Yellow - Other