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

Simplify failure deduplication in looponfail #544

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lgeiger
Copy link
Contributor

@lgeiger lgeiger commented Jun 20, 2020

This PR simplifies deduplication of failures in looponfail using set instead of iterating over all failures. This should simplify the code and in general might be a bit faster as well.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

i finally got to this

it looks good, it took a bit to follow back how it goes into the remotes trails variable

please rebase to master and add a changelog entry (or let me know if i may do that for you)

nice find!

@lgeiger
Copy link
Contributor Author

lgeiger commented Mar 30, 2022

Sorry for the late reply, I somehow missed the review. I rebased onto master, let's see what CI thinks.

if failure not in uniq_failures:
uniq_failures.append(failure)
self.failures = uniq_failures
self.failures = list(set(failures))
Copy link
Member

Choose a reason for hiding this comment

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

does this preserve order?

Copy link
Contributor Author

@lgeiger lgeiger Mar 31, 2022

Choose a reason for hiding this comment

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

It does not, I can change it to

Suggested change
self.failures = list(set(failures))
self.failures = list(dict.fromkeys(failures))

which will preserve the order if required.

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.

2 participants