sot / kadi

Chandra commands and events
https://sot.github.io/kadi
BSD 3-Clause "New" or "Revised" License
5 stars 3 forks source link

Fix unintended side effect of add_cmds #312

Closed taldcroft closed 5 months ago

taldcroft commented 5 months ago

Description

The CommandTable.add_cmds() method had a bug where it would update the original self table in place (using remove_rows) prior to using it for computing the new table with additional commands. I think the original idea of this method was for the whole thing to be in-place but that was inconvenient.

This fixes the issue by not using remove_rows and defines a new reference to the original commands which is filtered NOT in-place.

This also adds a careful test of the method which was previously lacking.

Interface impacts

None.

Testing

Unit tests

kadi/commands/tests/test_commands.py ..................................................................................... [ 37%] kadi/commands/tests/test_states.py ......................x.............................................x....................... [ 77%] [ 77%] kadi/commands/tests/test_validate.py .................... [ 86%] kadi/tests/test_events.py .......... [ 90%] kadi/tests/test_occweb.py ...................... [100%]

============================================= 227 passed, 2 xfailed in 143.25s (0:02:23) ==============================================


Independent check of unit tests by [REVIEWER NAME]
- [ ] [PLATFORM]:

### Functional tests
<!-- Describe and document results of any functional tests, otherwise leave the text below -->
No functional testing.
taldcroft commented 5 months ago

The docstring does not specify that, and instead uses colloquial language (Clip existing commands to the RLTT). It would have been nice to have it in the docstring).

I'll improve the docstring, that's a good point. There is already some PR cross-talk with the docstring here so I don't want to touch this now, praying to not generate merge conflicts when this gets merged.