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

Refactor config, add settings command #295

Merged
merged 44 commits into from
Jun 26, 2024

Conversation

jimkoen
Copy link
Contributor

@jimkoen jimkoen commented Feb 26, 2024

Fix #158

@jimkoen jimkoen requested a review from lionkor February 26, 2024 08:20
@jimkoen jimkoen self-assigned this Feb 26, 2024
@jimkoen jimkoen linked an issue Feb 26, 2024 that may be closed by this pull request
@jimkoen
Copy link
Contributor Author

jimkoen commented Feb 26, 2024

todo:

  • support concurrent access to config
  • cleanup in multiple places
  • change settings access to new settings.get... everywhere

@lionkor
Copy link
Contributor

lionkor commented Apr 20, 2024

@jimkoen what's the status on this?

@jimkoen
Copy link
Contributor Author

jimkoen commented Apr 20, 2024

@jimkoen what's the status on this?

I have a working version on my end where I need to fix 1-2 remaining build errors related to formatting of console output.

However as discussed privately I'm held up by exams, so this won't land before the week of the 29th of April.
If I'm holding something up, please tell me, I might be able to reschedule, but otherwise it would be very considerate if you could respect my exam schedule.

@lionkor
Copy link
Contributor

lionkor commented Apr 23, 2024

Thank you for the info

jimkoen added 13 commits May 7, 2024 18:16
into Settings by adding getters/setters

Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Make ComposedKey formattable by overloading fmt::format

Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
@jimkoen jimkoen force-pushed the 158-bug-running-settings-help-returns-nothing branch from 99c328d to ff81242 Compare May 7, 2024 16:17
jimkoen added 6 commits May 15, 2024 12:45
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
Signed-off-by: Lucca Jiménez Könings <development@jimkoen.com>
@jimkoen
Copy link
Contributor Author

jimkoen commented May 15, 2024

@lionkor Should be ready for review.

Unfortunately I cannot reproduce the test failures on Ubuntu 20.04 on my end. Tests work fine on FreeBSD (and other OS's as indicated by the Actions result). For now I think it'd make sense to start the review, so the issue doesn't become any more stale than it already is.

@jimkoen jimkoen marked this pull request as ready for review May 15, 2024 12:16
@jimkoen
Copy link
Contributor Author

jimkoen commented Jun 26, 2024

ready for review

@lionkor
Copy link
Contributor

lionkor commented Jun 26, 2024

And please run clang-format

@lionkor
Copy link
Contributor

lionkor commented Jun 26, 2024

A few things I noticed while testing:

  1. I get this when I run settings list: image

  2. When I enter an incomplete command I get a difficult to understand error: image

  3. AuthKey is gettable and settable: image

@lionkor
Copy link
Contributor

lionkor commented Jun 26, 2024

Also, we need to do this:

diff --git a/src/THeartbeatThread.cpp b/src/THeartbeatThread.cpp
index a7f37c6..3e6699c 100644
--- a/src/THeartbeatThread.cpp
+++ b/src/THeartbeatThread.cpp
@@ -40,8 +40,8 @@ void THeartbeatThread::operator()() {
     static std::chrono::high_resolution_clock::time_point LastUpdateReminderTime = std::chrono::high_resolution_clock::now();
     bool isAuth = false;
     std::chrono::high_resolution_clock::duration UpdateReminderTimePassed;
-    auto UpdateReminderTimeout = ChronoWrapper::TimeFromStringWithLiteral(Application::Settings.getAsString(Settings::Key::Misc_UpdateReminderTime));
     while (!Application::IsShuttingDown()) {
+        auto UpdateReminderTimeout = ChronoWrapper::TimeFromStringWithLiteral(Application::Settings.getAsString(Settings::Key::Misc_UpdateReminderTime));
         Body = GenerateCall();
         // a hot-change occurs when a setting has changed, to update the backend of that change.
         auto Now = std::chrono::high_resolution_clock::now();

We need to do this because UpdateReminderTime is (correctly) read+write, but it's only ever read once. That's broken :)

@lionkor
Copy link
Contributor

lionkor commented Jun 26, 2024

Also remove line ChronoWrapper.cpp:13 so it doesn't spam ;)

@lionkor
Copy link
Contributor

lionkor commented Jun 26, 2024

The error message when accessing an unknown key or category sucks, can we get that to say something like "that's not a setting ya doofus"

@lionkor
Copy link
Contributor

lionkor commented Jun 26, 2024

Remove ubuntu 20.04 support due to their outdated ahh compiler as well please

@jimkoen jimkoen force-pushed the 158-bug-running-settings-help-returns-nothing branch from 3e313bf to 08374b1 Compare June 26, 2024 12:12
Copy link
Contributor

@lionkor lionkor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@lionkor lionkor changed the title Refactor config, add settings command Refactor config, add settings command Jun 26, 2024
@lionkor lionkor merged commit 72022e3 into minor Jun 26, 2024
9 checks passed
@lionkor lionkor deleted the 158-bug-running-settings-help-returns-nothing branch June 26, 2024 12:24
@jimkoen jimkoen mentioned this pull request Jun 26, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Running settings help returns nothing
2 participants