greg/info-orbs
Watch
1
0
Fork
You've already forked info-orbs
 0
Code Issues 32 Pull requests 25 Projects Releases Packages Wiki Activity Actions

Rename HTTPClientWrapper to Taskmanager #218

Merged
dreed47 merged 16 commits from taskmanager into dev 2025-01-10 04:58:44 +00:00
Conversation 11 Commits 16 Files changed 14 +553 -270
dreed47 commented 2025-01-06 23:08:57 +00:00 (Migrated from github.com)
Copy link
  • Renamed HTTPClientWrapper to the more generic TaskManager
  • Added TaskFactory for creating task instances
  • Moved taskSemaphore to util/GlobalResources
  • Modified Weather and Stock widget to use TaskManager to perform it's http requests
- Renamed HTTPClientWrapper to the more generic TaskManager - Added TaskFactory for creating task instances - Moved taskSemaphore to util/GlobalResources - Modified Weather and Stock widget to use TaskManager to perform it's http requests
lyricnz commented 2025-01-07 07:57:16 +00:00 (Migrated from github.com)
Copy link

Please fix the clang-format issues in your PR. You can either run the megalinter (requires local docker) using npx mega-linter-runner or run clang-format directly (there's a config file in root dir).

+----SUMMARY--+--------------------------+---------------+-------+-------+--------+--------------+
| Descriptor  | Linter                   | Mode          | Files | Fixed | Errors | Elapsed time |
+-------------+--------------------------+---------------+-------+-------+--------+--------------+
| ✅ CPP      | clang-format             | list_of_files |    84 |     5 |      0 |        0.44s |
| ⚠️ MARKDOWN | markdownlint             | list_of_files |     5 |     0 |     37 |        0.46s |
| ✅ MARKDOWN | markdown-table-formatter | list_of_files |     5 |     0 |      0 |        0.19s |
+-------------+--------------------------+---------------+-------+-------+--------+--------------+

This will also remove some of the "noise" changes, like adding newlines to files

Please fix the clang-format issues in your PR. You can either run the megalinter (requires local docker) using `npx mega-linter-runner` or run clang-format directly (there's a config file in root dir). ``` +----SUMMARY--+--------------------------+---------------+-------+-------+--------+--------------+ | Descriptor | Linter | Mode | Files | Fixed | Errors | Elapsed time | +-------------+--------------------------+---------------+-------+-------+--------+--------------+ | ✅ CPP | clang-format | list_of_files | 84 | 5 | 0 | 0.44s | | ⚠️ MARKDOWN | markdownlint | list_of_files | 5 | 0 | 37 | 0.46s | | ✅ MARKDOWN | markdown-table-formatter | list_of_files | 5 | 0 | 0 | 0.19s | +-------------+--------------------------+---------------+-------+-------+--------+--------------+ ``` This will also remove some of the "noise" changes, like adding newlines to files
👍 1
lyricnz commented 2025-01-07 07:58:26 +00:00 (Migrated from github.com)
Copy link

Humbly, I suggest not removing comments from firmware/src/widgets/weatherwidget/WeatherWidget.cpp as part of your PR: this makes the change bigger, and is unrelated to your objective.

Humbly, I suggest not removing comments from firmware/src/widgets/weatherwidget/WeatherWidget.cpp as part of your PR: this makes the change bigger, and is unrelated to your objective.
dreed47 commented 2025-01-07 11:42:16 +00:00 (Migrated from github.com)
Copy link

Humbly, I suggest not removing comments from firmware/src/widgets/weatherwidget/WeatherWidget.cpp as part of your PR: this makes the change bigger, and is unrelated to your objective.

Sorry about that. It was unintentional and happened when I re-rolled

> Humbly, I suggest not removing comments from firmware/src/widgets/weatherwidget/WeatherWidget.cpp as part of your PR: this makes the change bigger, and is unrelated to your objective. Sorry about that. It was unintentional and happened when I re-rolled
dreed47 commented 2025-01-07 13:38:25 +00:00 (Migrated from github.com)
Copy link

Please fix the clang-format issues in your PR. You can either run the megalinter (requires local docker) using npx mega-linter-runner or run clang-format directly (there's a config file in root dir).

+----SUMMARY--+--------------------------+---------------+-------+-------+--------+--------------+
| Descriptor  | Linter                   | Mode          | Files | Fixed | Errors | Elapsed time |
+-------------+--------------------------+---------------+-------+-------+--------+--------------+
| ✅ CPP      | clang-format             | list_of_files |    84 |     5 |      0 |        0.44s |
| ⚠️ MARKDOWN | markdownlint             | list_of_files |     5 |     0 |     37 |        0.46s |
| ✅ MARKDOWN | markdown-table-formatter | list_of_files |     5 |     0 |      0 |        0.19s |
+-------------+--------------------------+---------------+-------+-------+--------+--------------+

This will also remove some of the "noise" changes, like adding newlines to files

I typically run format on save but I think this was a result of me re-rolling and messing with the files in another editor.

> Please fix the clang-format issues in your PR. You can either run the megalinter (requires local docker) using `npx mega-linter-runner` or run clang-format directly (there's a config file in root dir). > > ``` > +----SUMMARY--+--------------------------+---------------+-------+-------+--------+--------------+ > | Descriptor | Linter | Mode | Files | Fixed | Errors | Elapsed time | > +-------------+--------------------------+---------------+-------+-------+--------+--------------+ > | ✅ CPP | clang-format | list_of_files | 84 | 5 | 0 | 0.44s | > | ⚠️ MARKDOWN | markdownlint | list_of_files | 5 | 0 | 37 | 0.46s | > | ✅ MARKDOWN | markdown-table-formatter | list_of_files | 5 | 0 | 0 | 0.19s | > +-------------+--------------------------+---------------+-------+-------+--------+--------------+ > ``` > > This will also remove some of the "noise" changes, like adding newlines to files I typically run format on save but I think this was a result of me re-rolling and messing with the files in another editor.
👍 1
lyricnz (Migrated from github.com) reviewed 2025-01-09 22:40:17 +00:00
docs/TaskManager Developer Guide.md
@ -0,0 +1,117 @@
# TaskFactory API Documentation
lyricnz (Migrated from github.com) commented 2025-01-09 22:40:16 +00:00
Copy link

Probably no need to document internal functions (in markdown) - code comments should be sufficient?

Probably no need to document internal functions (in markdown) - code comments should be sufficient?
lyricnz (Migrated from github.com) reviewed 2025-01-09 22:41:34 +00:00
firmware/src/core/utils/TaskFactory.cpp
@ -0,0 +1,55 @@
#include "TaskFactory.h"
lyricnz (Migrated from github.com) commented 2025-01-09 22:41:33 +00:00
Copy link

Should this be httpGetTask (as compared to POST PUT etc)?

Should this be httpGetTask (as compared to POST PUT etc)?
lyricnz (Migrated from github.com) reviewed 2025-01-09 22:43:29 +00:00
firmware/src/core/utils/TaskManager.cpp
@ -0,0 +29,4 @@
}
bool TaskManager::addTask(std::unique_ptr<Task> task) {
if (isUrlInQueue(task->url)) {
lyricnz (Migrated from github.com) commented 2025-01-09 22:43:29 +00:00
Copy link

Should we emit a message about suppressing duplicate requests? technically they might not be duplicate based on URL if they have different post-processing (but that's super edge-case).

Should we emit a message about suppressing duplicate requests? _technically_ they might not be duplicate based on URL if they have different post-processing (but that's super edge-case).
lyricnz commented 2025-01-09 22:46:02 +00:00 (Migrated from github.com)
Copy link

I'm mostly good with this PR, subject to a couple of minor points. Does all the HTTP code use this path - or do some need to be updated to use it? (and the old/alternate path removed)

I'm mostly good with this PR, subject to a couple of minor points. Does all the HTTP code use this path - or do some need to be updated to use it? (and the old/alternate path removed)
dreed47 commented 2025-01-09 23:45:10 +00:00 (Migrated from github.com)
Copy link

I'm mostly good with this PR, subject to a couple of minor points. Does all the HTTP code use this path - or do some need to be updated to use it? (and the old/alternate path removed)

Not all http code use this yet, only weather and stocks. ParqetWidget needs updated to use it but I think will take a specific http request for handling http.getstream. I also haven't looked at any of the webdataWidget stuff but I assume that could be converted to use this as well. It was just too big to try to change all those modules in one change.

> I'm mostly good with this PR, subject to a couple of minor points. Does all the HTTP code use this path - or do some need to be updated to use it? (and the old/alternate path removed) Not all http code use this yet, only weather and stocks. ParqetWidget needs updated to use it but I think will take a specific http request for handling http.getstream. I also haven't looked at any of the webdataWidget stuff but I assume that could be converted to use this as well. It was just too big to try to change all those modules in one change.
dreed47 (Migrated from github.com) reviewed 2025-01-09 23:52:45 +00:00
docs/TaskManager Developer Guide.md
@ -0,0 +1,117 @@
# TaskFactory API Documentation
dreed47 (Migrated from github.com) commented 2025-01-09 23:52:45 +00:00
Copy link

im fine with removing the doc file. Just seems like maybe theres a broader need for a general developer guide or a how to contribute guide. I'll remove this file though

im fine with removing the doc file. Just seems like maybe theres a broader need for a general developer guide or a how to contribute guide. I'll remove this file though
lyricnz (Migrated from github.com) reviewed 2025-01-10 01:38:06 +00:00
docs/TaskManager Developer Guide.md
@ -0,0 +1,117 @@
# TaskFactory API Documentation
lyricnz (Migrated from github.com) commented 2025-01-10 01:38:05 +00:00
Copy link

The file is fine! (good even) I was just commenting about httpTask() - an internal function. Sorry if I was unclear

The file is fine! (good even) I was just commenting about httpTask() - an internal function. Sorry if I was unclear
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something isn't working

  
conflicts

   
documentation
Improvements or additions to documentation

  
DO NOT MERGE
Work in progress not ready to be considered for merging

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
good first issue
Good for newcomers

  
hardware
Problems or ideas about the hardware

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
bug
conflicts
documentation
DO NOT MERGE
duplicate
enhancement
good first issue
hardware
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
greg/info-orbs!218
No description provided.