coderabbitai[bot]
commented
2 months ago Walkthrough
## Walkthrough The recent changes involve the removal of the `svList` member from the `FairModule` class, which was not intended for public API exposure. Additionally, a comment was added to the `fAllSensitiveVolumes` variable in `FairModule.h` to clarify its internal use for containing all sensitive volumes in a simulation session. ## Changes | File | Change Summary | |-----------------------------------------|-------------------------------------------------------------------------------------------------------------------| | `CHANGELOG.md` | Documented the removal of `FairModule::svList` and clarified the use of `fAllSensitiveVolumes` in `FairModule.h`. | | `fairroot/base/sim/FairModule.h` | Removed `svList` from `FairModule` and added a comment to `fAllSensitiveVolumes` to specify its internal use. |Recent review details
**Configuration used: CodeRabbit UI** **Review profile: CHILL**Commits
Files that changed from the base of the PR and between 32bb52f41d5945acf128dcb3c712fa91fe2b5103 and 96fec1eab3a2f7a9ca2698b5d1cab94788486eb7.Files selected for processing (2)
* CHANGELOG.md (1 hunks) * fairroot/base/sim/FairModule.h (1 hunks)Files skipped from review due to trivial changes (1)
* fairroot/base/sim/FairModule.hAdditional context used
LanguageTool
CHANGELOG.md
[style] ~52-~52: ‘was in conflict with’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_WAS_IN_CONFLICT_WITH) Context: ...pped build switch `BUILD_UNITTESTS`, it was in conflict with the CMake standard switch [`BUILD_T... --- [formatting] ~147-~147: Consider inserting a comma after an introductory phrase for better readability. (IN_THAT_CASE_COMMA) Context: ...es * Allow running without output sink. In this case even persistent branches would not be s... --- [uncategorized] ~155-~155: A comma may be missing after the conjunctive/linking adverb ‘Also’. (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA) Context: ... the Base into the Online library. * Also the MBS/LMD related source classes were... --- [grammar] ~157-~157: With the singular noun ‘one’, use the verb “is”. (SINGULAR_NOUN_VERB_AGREEMENT) Context: ...ne library. * If one of these classes are used in your code, you need to link... --- [style] ~175-~175: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE) Context: ... being set, rewrite it accordingly. * If your code has specific C++ needs, consi... --- [misspelling] ~178-~178: This word is normally spelled as one. (EN_COMPOUNDS_ANTI_PATTERN) Context: ...`USE_PATH_INFO` CMake option. It was an anti-pattern. Don't use it! * Dropped `ROOT_GENERATE... --- [grammar] ~194-~194: The word “opt-in” is a noun. The verb is spelled with a space. (NOUN_VERB_CONFUSION) Context: ...on Boost.Serialization. The user must opt-in to the previously baseline Boost serial... --- [grammar] ~194-~194: You used an adverb (‘previously’) instead of an adjective, or a noun (‘baseline’) instead of another adjective. (A_RB_NN) Context: ...ialization. The user must opt-in to the previously baseline Boost serialization support by includ... --- [typographical] ~207-~207: The conjunction “so that” does not have a comma in front. (SO_THAT_UNNECESSARY_COMMA) Context: ...y require some API, please file an issue, so that we can see how to handle this. * Depre... --- [uncategorized] ~219-~219: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) Context: ...tion`. * `FairRadMapManager::Instance`, `FairRadLenManager::Instance` * `Fair... --- [uncategorized] ~223-~223: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) Context: ...thing. * `FairTask::*InputPersistance`: There's no getter for `fInputPersis... --- [uncategorized] ~230-~230: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) Context: ...ConstructASCIIGeometry(T*, TString, U*)`, use `FairModule::ConstructASCIIGeom... --- [uncategorized] ~256-~256: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA) Context: ...se` style naming. For those using the targets this change will not be visible. * Test... --- [formatting] ~257-~257: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause. (COMMA_BEFORE_BECAUSE) Context: ...ing Geant3 have been disabled by default, because those tests have a probability > 0 fo... --- [style] ~259-~259: The word ‘anyways’ is informal American English. Did you mean “anyway”? (ANYWAYS) Context: ... for failing. If you want to run them anyways, pass `-DENABLE_GEANT3_TESTING=ON` to...
Markdownlint
CHANGELOG.md
114-114: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) Headings should be surrounded by blank lines --- 126-126: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) Headings should be surrounded by blank lines --- 134-134: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) Headings should be surrounded by blank lines --- 140-140: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) Headings should be surrounded by blank lines --- 146-146: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) Headings should be surrounded by blank lines --- 151-151: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) Headings should be surrounded by blank lines --- 239-239: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings) Headings should be surrounded by blank lines --- 30-30: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 33-33: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 162-162: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 164-164: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 165-165: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 167-167: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 168-168: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 172-172: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 196-196: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 199-199: null (MD031, blanks-around-fences) Fenced code blocks should be surrounded by blank lines --- 115-115: null (MD032, blanks-around-lists) Lists should be surrounded by blank lines --- 127-127: null (MD032, blanks-around-lists) Lists should be surrounded by blank lines --- 135-135: null (MD032, blanks-around-lists) Lists should be surrounded by blank lines --- 141-141: null (MD032, blanks-around-lists) Lists should be surrounded by blank lines --- 147-147: null (MD032, blanks-around-lists) Lists should be surrounded by blank lines --- 152-152: null (MD032, blanks-around-lists) Lists should be surrounded by blank lines --- 240-240: null (MD032, blanks-around-lists) Lists should be surrounded by blank lines --- 14-14: null (MD034, no-bare-urls) Bare URL used --- 16-16: null (MD034, no-bare-urls) Bare URL used --- 91-91: null (MD034, no-bare-urls) Bare URL used --- 110-110: null (MD034, no-bare-urls) Bare URL used
Additional comments not posted (1)
CHANGELOG.md (1)
`77-78`: Document the removal of `FairModule::svList` clearly. This entry clearly communicates the removal of `FairModule::svList` and its intended non-public API status, aligning with the PR's objectives.
ChristianTackeGSI
(prompted by https://github.com/FairRootGroup/FairRoot/pull/1500#issuecomment-2152317249)
The FairModule::svList API was dropped in 19.0. It was thought of as an internal API anyway. But let's at least document
See: 54ad232ffe416e9c650b3d0d4aadce7b48e48746
(Should be backported to 19.0 after it's merged.)
Checklist: