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

Added fix for unstable element index (issue #223) #224

Merged
merged 1 commit into from
Nov 5, 2022

Conversation

erbsenmann
Copy link

Hi, I solved the issue described in #223 using this patch locally. With this adaption I was able to run my filter as expected.
Also added a smaller test than in the ticket.

Idea is to fill the index position in the ListContainer along with the parent and location instead of deriving it.
Like this eq on element can be left untouched.

Copy link
Owner

@sergiocorreia sergiocorreia left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, as well as for spotting the bug which felt quite subtle on a first pass. (Also, apologies for not spotting the issue beforehand, I somehow missed the email from Github).

The PR looks great, but is it possible to do a minor edit to the version.py file and bump it to 2.2.5? In that way, the github runner will be able to push it to PyPI (I tried to search for a way to edit the PR as a reviewer but can't seem to find one).

Thanks again!

@ickc
Copy link
Collaborator

ickc commented Nov 5, 2022

is it possible to do a minor edit to the version.py file and bump it to 2.2.5?

I'd advise against this as so far this PR is standalone about 1 issue. Version bumping is about releasing a package which is not part of this issue or PR's job.

Put it in other words, it would be confusing if all PRs are asked to bump the version because it is possible that only after a few PRs there's a version release. And since that cannot be asked to all PRs, it is equally confusing to just ask some of them doing it.

@sergiocorreia
Copy link
Owner

Fair enough!

@sergiocorreia sergiocorreia merged commit ff94a0c into sergiocorreia:master Nov 5, 2022
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

3 participants