-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add option for including context before and after a label #331
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for your contribution. I am not sure your code does what the issue asks for. See the below more specific comments for that.
As per the Quality Standards outlined in the Contribution Guidelines I would (also) ask you to please include some tests for this added functionality.
fa4d5f8
to
8683b52
Compare
I added a test to verify the implementation, and fixed the code in the view logic! 😄 |
- use checked_sub in favour of unclear usize::min - remove `rev` because it does the opposite of what it was supposed to do - add some comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wraps this up. Thanks again for your contribution!
…#331) * Add option for including context before and after a label * simplify logic: - use checked_sub in favour of unclear usize::min - remove `rev` because it does the opposite of what it was supposed to do - add some comments Co-authored-by: Johann150 <johann.galle@protonmail.com>
When will this be released? |
This PR implements #29!
I'm not sure that this is the best way to implement this, as i'm not all that deep into the codebase. If necessary, i'd appreciate any suggestions or tips on how to improve this!