-
Notifications
You must be signed in to change notification settings - Fork 31
Move PID look up tables into global pid_lut file #1745
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
Conversation
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.
This should be fine to do.
Capybara summary for PR 1745
|
What's the plan for this PR? Since the conflicts are likely due to the clang-format or clang-tidy PR, I can offer to resolve them, but only if there's a plan to merge this. |
I still think it makes the data flow clearer. I will resolve the conflicts myself but might add another intermediate PR to move all of the remaining factories out of src/global into src/factories. |
Hmm... is GitHub confused about e2dfefd, or did something go wrong in the update/rebase? I see the entire repository reformat in this PR again. |
Something probably went wrong with the rebase. I have to admit I haven't quite got my head around how to rebase/merge safely, I think this happened because I earlier merged origin/main and now have tried to rebase. |
It's not a lot of work so I can just create a new clean branch and PR to avoid the mess. Unless you have a better idea. |
If it's just the first 3 commits (?), you could possibly run something like:
There will be conflicts, but hopefully manageable. |
e2dfefd
to
df3cb4a
Compare
Thanks, hopefully that's much better |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
62b1eff
to
63e7e34
Compare
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.
Looks good to me. Waiting for diffs.
Should I ignore iwyu asking for both:
I'll wait for the diffs before drawing conclusions about the tests and benchmarks not using the right libraries |
I looked into this only briefly, but I think iwyu is correct here in suggesting that both should be included. The JApplicationFwd.h is an incomplete forwarding header, so some things need JApplication.h as well. |
Diff looks good. |
Briefly, what does this PR introduce?
Moves all of the PID LUT factories out of their detector plugin into the global pid plugin. Also moves PID factory files into the factories directory.
Maybe a matter of personal taste but this would make the data flow through the PID algorithms more transparent.
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
No
Does this PR change default behavior?
No