Skip to content

Revert "Merge pull request #3379 from Open-Systems-Pharmacology/3014#3442

Merged
msevestre merged 1 commit intodevelopfrom
revert-3014-multiple-dose-from-develop
Mar 9, 2026
Merged

Revert "Merge pull request #3379 from Open-Systems-Pharmacology/3014#3442
msevestre merged 1 commit intodevelopfrom
revert-3014-multiple-dose-from-develop

Conversation

@benjaperez1983
Copy link
Contributor

@benjaperez1983 benjaperez1983 commented Mar 9, 2026

Fixes #3014

Description

Undo the changes from #3014 to develop branch

Type of change

Please mark relevant options with an x in the brackets.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires documentation changes (link at least one user or developer documentation issue):
  • Algorithm update - updates algorithm documentation/questions/answers etc.
  • Other (please describe):

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Integration tests
  • Unit tests
  • Manual tests
  • No tests required

Reviewer checklist

Mark everything that needs to be checked before merging the PR.

  • Check if the code is well documented
  • Check if the behavior is what is expected
  • Check if the code is well tested
  • Check if the code is readable and well formatted
  • Additional checks (document below if any)
  • Check if documentation update issue(s) are created if the option This change requires a documentation update above is selected

Screenshots (if appropriate):

Questions (if appropriate):

Summary by CodeRabbit

  • Refactor
    • Internal improvements to application naming logic for enhanced code clarity and maintainability.

…multiple-dose-application-naming"

This reverts commit e72e7d8, reversing
changes made to f8c1773.
@coderabbitai
Copy link

coderabbitai bot commented Mar 9, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 09485e94-0dde-4d46-b4c0-26a5462b86e8

📥 Commits

Reviewing files that changed from the base of the PR and between 1999ab6 and 06b00ed.

📒 Files selected for processing (1)
  • src/PKSim.Core/Services/EventBuildingBlockCreator.cs

📝 Walkthrough

Walkthrough

The change modifies application naming in the event building block creation service by replacing a multi-step padding-based naming scheme with a simpler 1-based index approach. The iteration pattern is refactored from mapping to ToList() with indexing to direct MapFrom(...).Each(...) calls, while preserving per-item processing logic.

Changes

Cohort / File(s) Summary
Application Naming Refactor
src/PKSim.Core/Services/EventBuildingBlockCreator.cs
Replaces padding-based name generation with direct 1-based indexing (index + 1). Removes width calculation and zero-padding logic. Refactors iteration from ToList() indexing to MapFrom(...).Each(...). Adds clarifying comment about 1-based nomenclature.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A simpler path for naming apps so fine,
No padding tricks, just index plus one divine,
Application_001 through countless more,
Now sorted neat, much cleaner than before!

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR title indicates reverting issue #3014, but the code changes show removal of the padding-based index naming scheme (e.g., Application_001) that was implemented to address #3014's requirements. Clarify whether this PR should revert #3014 changes entirely or if the implementation needs refinement. The current changes contradict the stated objective of maintaining sequential naming for better sorting.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: reverting a specific pull request (#3379/issue #3014) related to multiple-dose application naming.
Out of Scope Changes check ✅ Passed The changes in EventBuildingBlockCreator.cs are directly related to reverting the application naming logic from issue #3014, with no unrelated modifications present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch revert-3014-multiple-dose-from-develop

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@msevestre
Copy link
Member

@ALL: let's make sure we add tests in the future for domain changes. IT's not becuase it was not tested before that it should not be tested when making changes to the domain

@msevestre msevestre self-requested a review March 9, 2026 15:14
@msevestre msevestre merged commit 0a83906 into develop Mar 9, 2026
6 checks passed
@msevestre msevestre deleted the revert-3014-multiple-dose-from-develop branch March 9, 2026 15:14
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.

Multiple dose application naming

2 participants