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

Fix indexing for real this time (instead of 0, use len(...) -1) #335

Merged
merged 1 commit into from
Oct 21, 2018

Conversation

kratsg
Copy link
Contributor

@kratsg kratsg commented Oct 21, 2018

Description

See #333. This is fixing it according to @lukasheinrich 's comment.

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR

@lukasheinrich
Copy link
Contributor

Sorry for not catching this earlier

@kratsg
Copy link
Contributor Author

kratsg commented Oct 21, 2018

Sorry for not catching this earlier

It's ok. I don't even think there's a way to enforce a test for this (especially since our tests didn't catch it). So maybe we just validate shapesys/staterror better moving forward.

@lukasheinrich
Copy link
Contributor

lukasheinrich commented Oct 21, 2018

Tbh I think the mask will enforce the right value just before returning in apply

@kratsg
Copy link
Contributor Author

kratsg commented Oct 21, 2018

Tbh I think the mask will enforce the rift value just before returning in apply

I thought the same thing too, but I'm not fully sure... That's what I initially set it to 0 thinking it'll just be masked out anyway...

@lukasheinrich lukasheinrich merged commit 700033d into master Oct 21, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.714% when pulling 841dd00 on fix/indexingCombinedMods into 0fdfcc3 on master.

@kratsg kratsg deleted the fix/indexingCombinedMods branch October 21, 2018 20:30
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.

3 participants