Refactor Scanner logic out of AroundBlockScan introduce history #192
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AroundBlockScan started as a utility class that was meant to be used as a DSL for scanning and making new blocks. As logic got added to this class it became hard to reason about what exactly is being mutated when. I pulled the scanning logic out into it's own class which gives us a clean separation of concerns. This allowed me to remove a lot of accessors that aren't core to the logic provided by AroundBlockScan. In addition to this refactor the ScanHistory class can snapshot a scan. This allows us to be more aggressive with scans in the future as we can now snapshot and rollback if it didn't turn out the way we wanted. The change comes with a minor perf impact: before: 5.092678 0.104299 5.196977 ( 5.226494) after: 5.128536 0.099871 5.228407 ( 5.249542) This represents a 0.996x change in speed (where 1x would be no change and 2x would be twice as fast). This is a 0.38% decrease in performance which is negligible. It's likely coming from the extra blocks being created while scanning. This is negligible and if the history feature works well we might be able to make better block decisions which is means fewer calls to ripper which is the biggest bottleneck. While this doesn't fix #187 it's a good intermediate step that will hopefully make working on that issue easier.
schneems
added a commit
that referenced
this pull request
May 3, 2023
- Add tests to ScanHistory - Add docs to ScanHistory - Maybe refactor some logic from AroundBlock Scan to somewhere else Reported in #187 this code: ``` class Foo def foo if cond? foo else # comment end end # ... def bar if @recv end_is_missing_here end end ``` Triggers an incorrect suggestion: ``` Unmatched keyword, missing `end' ? 1 class Foo 2 def foo > 3 if cond? > 5 else 8 end 16 end ``` Part of the issue is that while scanning we're using newlines to determine when to stop and pause. This is useful for determining logically smaller chunks to evaluate but in this case it causes us to pause before grabbing the "end" that is right below the newline. This problem is similar to #179. However in the case of expanding same indentation "neighbors" I want to always grab all empty values at the end of the scan. I don't want to do that when changing indentation levels as it affects scan results. There may be some way to normalize this behavior between the two, but the tests really don't like that change. To fix this issue for expanding against different indentation I needed a way to first try and grab any additional newlines with the ability to rollback that guess. In #192 I experimented with decoupling scanning from the AroundBlockScan logic. It also added the ability to take a snapshot of the current scanner and rollback to prior changes. With this ability in place now we: - Grab extra empties before looking at the above/below line for the matching keyword/end statement - If there's a match, grab it - If there's no match, discard the newlines we picked up in the evaluation That solves the issue.
schneems
added a commit
that referenced
this pull request
May 3, 2023
Reported in #187 this code: ``` class Foo def foo if cond? foo else # comment end end # ... def bar if @recv end_is_missing_here end end ``` Triggers an incorrect suggestion: ``` Unmatched keyword, missing `end' ? 1 class Foo 2 def foo > 3 if cond? > 5 else 8 end 16 end ``` Part of the issue is that while scanning we're using newlines to determine when to stop and pause. This is useful for determining logically smaller chunks to evaluate but in this case it causes us to pause before grabbing the "end" that is right below the newline. This problem is similar to #179. However in the case of expanding same indentation "neighbors" I want to always grab all empty values at the end of the scan. I don't want to do that when changing indentation levels as it affects scan results. There may be some way to normalize this behavior between the two, but the tests really don't like that change. To fix this issue for expanding against different indentation I needed a way to first try and grab any additional newlines with the ability to rollback that guess. In #192 I experimented with decoupling scanning from the AroundBlockScan logic. It also added the ability to take a snapshot of the current scanner and rollback to prior changes. With this ability in place now we: - Grab extra empties before looking at the above/below line for the matching keyword/end statement - If there's a match, grab it - If there's no match, discard the newlines we picked up in the evaluation That solves the issue.
schneems
added a commit
that referenced
this pull request
May 3, 2023
Reported in #187 this code: ``` class Foo def foo if cond? foo else # comment end end # ... def bar if @recv end_is_missing_here end end ``` Triggers an incorrect suggestion: ``` Unmatched keyword, missing `end' ? 1 class Foo 2 def foo > 3 if cond? > 5 else 8 end 16 end ``` Part of the issue is that while scanning we're using newlines to determine when to stop and pause. This is useful for determining logically smaller chunks to evaluate but in this case it causes us to pause before grabbing the "end" that is right below the newline. This problem is similar to #179. However in the case of expanding same indentation "neighbors" I want to always grab all empty values at the end of the scan. I don't want to do that when changing indentation levels as it affects scan results. There may be some way to normalize this behavior between the two, but the tests really don't like that change. To fix this issue for expanding against different indentation I needed a way to first try and grab any additional newlines with the ability to rollback that guess. In #192 I experimented with decoupling scanning from the AroundBlockScan logic. It also added the ability to take a snapshot of the current scanner and rollback to prior changes. With this ability in place now we: - Grab extra empties before looking at the above/below line for the matching keyword/end statement - If there's a match, grab it - If there's no match, discard the newlines we picked up in the evaluation That solves the issue.
schneems
added a commit
that referenced
this pull request
May 3, 2023
Reported in #187 this code: ``` class Foo def foo if cond? foo else # comment end end # ... def bar if @recv end_is_missing_here end end ``` Triggers an incorrect suggestion: ``` Unmatched keyword, missing `end' ? 1 class Foo 2 def foo > 3 if cond? > 5 else 8 end 16 end ``` Part of the issue is that while scanning we're using newlines to determine when to stop and pause. This is useful for determining logically smaller chunks to evaluate but in this case it causes us to pause before grabbing the "end" that is right below the newline. This problem is similar to #179. However in the case of expanding same indentation "neighbors" I want to always grab all empty values at the end of the scan. I don't want to do that when changing indentation levels as it affects scan results. There may be some way to normalize this behavior between the two, but the tests really don't like that change. To fix this issue for expanding against different indentation I needed a way to first try and grab any additional newlines with the ability to rollback that guess. In #192 I experimented with decoupling scanning from the AroundBlockScan logic. It also added the ability to take a snapshot of the current scanner and rollback to prior changes. With this ability in place now we: - Grab extra empties before looking at the above/below line for the matching keyword/end statement - If there's a match, grab it - If there's no match, discard the newlines we picked up in the evaluation That solves the issue.
schneems
added a commit
that referenced
this pull request
May 3, 2023
Reported in #187 this code: ``` class Foo def foo if cond? foo else # comment end end # ... def bar if @recv end_is_missing_here end end ``` Triggers an incorrect suggestion: ``` Unmatched keyword, missing `end' ? 1 class Foo 2 def foo > 3 if cond? > 5 else 8 end 16 end ``` Part of the issue is that while scanning we're using newlines to determine when to stop and pause. This is useful for determining logically smaller chunks to evaluate but in this case it causes us to pause before grabbing the "end" that is right below the newline. This problem is similar to #179. However in the case of expanding same indentation "neighbors" I want to always grab all empty values at the end of the scan. I don't want to do that when changing indentation levels as it affects scan results. There may be some way to normalize this behavior between the two, but the tests really don't like that change. To fix this issue for expanding against different indentation I needed a way to first try and grab any additional newlines with the ability to rollback that guess. In #192 I experimented with decoupling scanning from the AroundBlockScan logic. It also added the ability to take a snapshot of the current scanner and rollback to prior changes. With this ability in place now we: - Grab extra empties before looking at the above/below line for the matching keyword/end statement - If there's a match, grab it - If there's no match, discard the newlines we picked up in the evaluation That solves the issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
AroundBlockScan started as a utility class that was meant to be used as a DSL for scanning and making new blocks. As logic got added to this class it became hard to reason about what exactly is being mutated when. I pulled the scanning logic out into it's own class which gives us a clean separation of concerns. This allowed me to remove a lot of accessors that aren't core to the logic provided by AroundBlockScan.
In addition to this refactor the ScanHistory class can snapshot a scan. This allows us to be more aggressive with scans in the future as we can now snapshot and rollback if it didn't turn out the way we wanted.
The change comes with a minor perf impact:
before: 5.092678 0.104299 5.196977 ( 5.226494)
after: 5.128536 0.099871 5.228407 ( 5.249542)
This represents a 0.996x change in speed (where 1x would be no change and 2x would be twice as fast). This is a 0.38% decrease in performance which is negligible. It's likely coming from the extra blocks being created while scanning. This is negligible and if the history feature works well we might be able to make better block decisions which is means fewer calls to ripper which is the biggest bottleneck.