Skip to content
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

DM-42054: Remove limit of 1 for getCommandsDuringEvent #83

Merged
merged 5 commits into from Mar 1, 2024

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

if timeFormat not in ['pandas', 'astropy', 'python']:
raise ValueError(f"format must be one of 'pandas', 'astropy' or 'python', not {timeFormat=}")

commands = list(ensure_iterable(commands))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "listify" here? Can't the iterable be directly used in the for loop below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate that too, but ensure_iterable will just return an exhaustible generator if it got one of those, and I hate accidentally consuming things when looping over them to find they're now gone, so just tend to do that for safety when I know the list is small, mainly just out of habit. It's not necessary here, but it's also probably less harmful than not doing it, on balance.

log = logging.getLogger(__name__)

commands = ensure_iterable(commands)
commands = list(ensure_iterable(commands))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question as above...

Copy link

@fritzm fritzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM at a coding level, one minor question. You may still want Tiago's eyes for possible impact to the broader codebase?

@mfisherlevine
Copy link
Contributor Author

Thanks. Should be fine though, this can't affect anything in Tiago-town (expect making it easier for him to use this code to debug things 🙂)

@mfisherlevine mfisherlevine merged commit 7c46278 into main Mar 1, 2024
1 check passed
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.

None yet

2 participants