Skip to content

Fixed bug where line breakpoint would use incorrectly character #490

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

Merged
merged 10 commits into from
Jan 19, 2022

Conversation

AndreasArvidsson
Copy link
Member

Line level breakpoints are always on character 0. This new implementation is consistent with the built in vscode breakpoint functions in regard to line and inline breakpoints.

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

This looks good but I am definitely going to break this by accident when I implement object-oriented targets 😅. It shouldn't be too bad to add a test to capture this fix, should it?

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Jan 17, 2022

Adding break points to the test is definitely something we want to do but I do think it's an pr in itself. May be something to add to the todo when/before object orient the targets are implemented to make sure that we don't don't have any regression?

@pokey
Copy link
Member

pokey commented Jan 17, 2022

I don't think it should be too hard. Just a manual test like the one you did for #382, no? Ie run the add breakpoint command, then list editor breakpoints. I'd think the actual content (ignoring test setup boilerplate) would be like 10–15 lines. But maybe I'm missing something?

@pokey pokey added the bug Something isn't working label Jan 17, 2022
@AndreasArvidsson
Copy link
Member Author

Ok I was thinking line breaks for the test recorder. A one of can of course be made.

@pokey
Copy link
Member

pokey commented Jan 17, 2022

Ok I was thinking line breaks for the test recorder. A one of can of course be made.

No that seems like overkill. Just one to test happy path and one to test the thing you just fixed is probably plenty. Can't imagine we'll be recording lots of breakpoint tests

@AndreasArvidsson
Copy link
Member Author

Tests added

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Nice test! I believe your code is also careful to make sure it doesn't move the end of the target to the start of the line when it is doing a removal, so might be good to add a quick test to ensure that using "break air" for removal works properly, lest someone (me probably 😅) moves that if statement upwards in an effort to "simplify". Make sense?

@pokey pokey mentioned this pull request Jan 18, 2022
@AndreasArvidsson
Copy link
Member Author

Removal tests added

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Looks good! I made a minor change to a unit test, but assuming that passes this one is good to go from my perspective 👍

@AndreasArvidsson AndreasArvidsson merged commit 0ca8def into main Jan 19, 2022
@AndreasArvidsson AndreasArvidsson deleted the lineBreakpoint branch January 19, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants