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

Update replacementPrefix for suggestions when returned multiple times #983

Merged
merged 4 commits into from Jul 23, 2018

Conversation

Projects
None yet
3 participants
@sadikovi
Copy link
Member

sadikovi commented Jul 21, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This PR adds isPrefixModified flag to a suggestion to distinguish between suggestions that originally had prefix set and ones that did not. Original issue was that suggestions get reused without resetting a prefix. This flag checks that we have modified prefix in the past, and so we need to reset to a new one.

For more detailed explanation see #975 (comment).

Alternate Designs

Returning a fresh copy of a suggestion every time provider.getSuggestions is called was one of the alternatives. It arguably could be a better approach, but I was trying to avoid extensive copying.

Benefits

Solves the issue when a suggestion will not be selected correctly. Potentially solves some other related issues.

Possible Drawbacks

None were observed.

Applicable Issues

#975.

@sadikovi

This comment has been minimized.

Copy link
Member Author

sadikovi commented Jul 21, 2018

@50Wliu @rsese Could you review this PR, please? Follows the discussion on the issue. Thanks!

sadikovi added some commits Jul 23, 2018

@rsese

This comment has been minimized.

Copy link
Member

rsese commented Jul 23, 2018

thanks @sadikovi! I'm not an engineer so can't do a code review but I think @queerviolet was happy to take a look 👓?

@queerviolet
Copy link
Contributor

queerviolet left a comment

👍

@queerviolet queerviolet merged commit c6cd614 into atom:master Jul 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sadikovi

This comment has been minimized.

Copy link
Member Author

sadikovi commented Jul 23, 2018

Thanks a lot @queerviolet!

@queerviolet

This comment has been minimized.

Copy link
Contributor

queerviolet commented Jul 23, 2018

Thanks for the PR! Looks good, merged.

@sadikovi sadikovi deleted the sadikovi:issue-975 branch Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.