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

Copy Phrase targetness bug #179

Merged
merged 1 commit into from
Oct 21, 2021
Merged

Copy Phrase targetness bug #179

merged 1 commit into from
Oct 21, 2021

Conversation

lawhead
Copy link
Collaborator

@lawhead lawhead commented Oct 20, 2021

Overview

This PR fixes the triggers.txt file output by the Copy Phrase task to correctly label targets.

The error was in the calculation for the next target in the helper function. This method took a parameter for the copy_phrase and one for the typed_letters and used this information to determine the target. However, prior to a recent refactor, the spelled text was updated before writing triggers. In the current code the triggers are written prior to the decision that updates spelled text. So the code was using the last letter of the spelled text rather than the next un-spelled letter.

Given the potential ambiguity here, I refactored the code to make the target_symbol a parameter. This was already being calculated by the Task for use in the session data.

Ticket

https://www.pivotaltracker.com/story/show/179811273

Contributions

  • Refactored helper to write the triggers data
  • Renamed the method to indicate that it was public
  • Updated tests

Test

  • Ran the unit tests
  • Ran the Copy Phrase task and analyzed the resulting triggers.txt file (along with the session.json file) to confirm that the correct labels were output.

@lawhead lawhead requested a review from tab-cmd October 20, 2021 00:21
@lawhead lawhead merged commit 283fb60 into 1.5.1 Oct 21, 2021
@lawhead lawhead deleted the targetness-bug branch October 21, 2021 16:22
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.

2 participants