refactor: no return for add* python helpers

[andiwand]

after working on the full chain doc I wondered why we do s = addSomething(s, ...) all the time. to me it looks kind of confusing as the reader might think s is changing every time we add something.

maybe we want to remove the return value of the add functions completely? @paulgessinger @timadye

[codecov]

Codecov Report

Merging #1404 (04917a1) into main (4e9aa53) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1404   +/-   ##
=======================================
  Coverage   48.59%   48.59%           
=======================================
  Files         380      380           
  Lines       20577    20577           
  Branches     9429     9429           
=======================================
  Hits         9999     9999           
  Misses       4093     4093           
  Partials     6485     6485           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[timadye]

Hi @andiwand,

This was originally proposed by @asalzburger in #1098, so we should get his feedback.

I think the idea is to emphasize that the Sequencer object is modified by the routine, even though it returns the same object. But maybe it is overkill to get the user/caller do it.

For myself, I don't have a strong opinion either way, so I wouldn't mind if it was changed - as long as it was changed consistently in all the examples and documentation.

Thanks,
Tim.

[andiwand]

hmm makes sense. I just wonder if the return value emphasizes this enough as you can easily skip it without any loss of functionality. I guess what would be nice is something like s.addSomething(...). we might be able to achieve this by wrapping the sequencer now that everything is in simulation.py or reconstruction.py?

[timadye]

That could be easier to use, but could make the implementation harder to understand. The aim with this interface is to make it simple to get started, but not to discourage people playing inside the add*() functions' code. I'll leave it to others to comment on this case, because I am sometimes similarly tempted to improve the interface.

[paulgessinger]

I would prefer not to wrap the sequencer. My original reason for having it as an argument an return value was that you could pass in None and the original run* function would create a default sequencer. I guess this is not relevant anymore with the add* functions.

I think my preferred solution would be to drop the return value consistently.

[andiwand]

I agree - I will draft these changes

[timadye]

Cool. Can you change the PR title, since this doesn't any more just affect the OOD.

[andiwand]

I made the changes @timadye. I left the None return type for now, with the hope that a linter might notify the consumer that a return value should not be expected

[timadye]

all looks good to me

[acts-project-service]

The backport to develop/v19.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-develop/v19.x develop/v19.x
# Navigate to the new working tree
cd .worktrees/backport-develop/v19.x
# Create a new branch
git switch --create backport-1404-to-develop/v19.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 afb91a583e6db641c9b39354c7494da5726b8681
# Push it to GitHub
git push --set-upstream origin backport-1404-to-develop/v19.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-develop/v19.x

Then, create a pull request where the base branch is develop/v19.x and the compare/head branch is backport-1404-to-develop/v19.x.