-
Notifications
You must be signed in to change notification settings - Fork 98
fix(sim) Add GetSaveCurrentEvent() to FairMCApplication #1605
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
base: dev
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughA new public getter method, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FairMCApplication
User->>FairMCApplication: Call SetSaveCurrentEvent(value)
FairMCApplication-->>FairMCApplication: Set fSaveCurrentEvent = value
User->>FairMCApplication: Call GetSaveCurrentEvent()
FairMCApplication-->>User: Return fSaveCurrentEvent
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fairroot/base/sim/FairMCApplication.h (1)
222-226
: Fix typo and add const qualifier for better design.The getter implementation is correct and follows good naming conventions. However, there are two minor improvements:
- Fix the typo "goint" → "going" in the documentation
- Add
const
qualifier since the method doesn't modify object stateApply this diff to improve the method:
- /** - * Returns the state of the flag deciding whether the current event - * is goint to be stored. - */ - Bool_t GetSaveCurrentEvent() { return fSaveCurrentEvent; } + /** + * Returns the state of the flag deciding whether the current event + * is going to be stored. + */ + Bool_t GetSaveCurrentEvent() const { return fSaveCurrentEvent; }The addition successfully addresses the PR objective of providing read access to the
fSaveCurrentEvent
flag and complements the existing setter nicely.
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.
Please update the copyright header of changed files to include the current year.
Allows the user to get the state of the fSaveCurrentEvent flag. Fixes issue FairRootGroup#1604.
Allows the user to get the state of the fMarkFill flag. Fixes issue FairRootGroup#1604.
Thanks! :-) Small bits…
Yes, you're "fixing" an issue. I'd call it resolving an issue. The issue IMHO is a feature request. P.S.: I hope the CI gets fixed at some point… |
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.
While you work on these, maybe move the initializers for fSaveCurrentEvent
and fMarkFill
to default member initializers?
/** | ||
* Returns the state of the flag deciding whether the current event | ||
* is goint to be stored. | ||
*/ |
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.
Fix the typo: "going"
* Returns the state of the flag deciding whether the current event | ||
* is goint to be stored. | ||
*/ | ||
bool GetSaveCurrentEvent() { return fSaveCurrentEvent; } |
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.
Mark this as const
bool GetSaveCurrentEvent() { return fSaveCurrentEvent; } | |
bool GetSaveCurrentEvent() const { return fSaveCurrentEvent; } |
@@ -154,6 +154,9 @@ class FairRun : public TNamed | |||
//** Mark/Unmark event to be filled into output. Default is TRUE. */ | |||
void MarkFill(Bool_t flag) { fMarkFill = flag; } | |||
|
|||
//** Get the state of the flag deciding whether the current event is goint to be stored. */ |
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.
Please use consistant Doxygen formatting everywhere.
Allows the user to get the state of the fSaveCurrentEvent flag.
Fixes issue #1604.
Checklist: