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

Fix incorrect parsing of placeholders that point to other tab stops. #267

Merged
merged 4 commits into from Apr 11, 2018

Conversation

Projects
None yet
5 participants
@savetheclocktower
Collaborator

savetheclocktower commented Apr 9, 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

#266 describes a regression with tab stop behavior when the placeholder contains a reference to another tab stop. This almost certainly happened as a result of #260, but there wasn't any test coverage for this particular usage.

Alternate Designs

The parser was incorrectly assuming that all the items it would receive for a particular rule would be represented as strings, and that it could thus join them all together. Tabstop references are represented as objects. So I wrote a couple of utility functions to ensure that adjacent strings would be merged together without incorrectly coercing objects.

If there's an easier way to do this with a PEG, I'm all for it. Parser grammars are not my strong suit.

Applicable Issues

#266

@savetheclocktower

This comment has been minimized.

Collaborator

savetheclocktower commented Apr 9, 2018

I can merge this myself, but I'll leave it open for a couple days in case other maintainers have feedback.

@savetheclocktower savetheclocktower referenced this pull request Apr 9, 2018

Closed

Nested tab stops now expand incorrectly #266

1 of 1 task complete
@rsese

This comment has been minimized.

Member

rsese commented Apr 9, 2018

Thanks @savetheclocktower, I'll ask the team to take a look in case anyone has feedback for you 🙇

@nathansobo

This comment has been minimized.

Contributor

nathansobo commented Apr 10, 2018

@joefitzgerald can you give this a spin and see if it fixes things?

@joefitzgerald

This comment has been minimized.

Member

joefitzgerald commented Apr 10, 2018

I have tested and can confirm that this restores the behavior that was present in v1.24.1.

@lee-dohm lee-dohm referenced this pull request Apr 10, 2018

Closed

Classname in Snippets broken after update. #249

1 of 1 task complete

@jasonrudolph jasonrudolph merged commit 66f86ea into atom:master Apr 11, 2018

2 checks passed

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

This comment has been minimized.

Contributor

nathansobo commented Apr 11, 2018

❤️❤️

@rsese rsese referenced this pull request Apr 16, 2018

Closed

Some snippets aren't moving correctly on Tab press #195

1 of 1 task complete

@savetheclocktower savetheclocktower deleted the savetheclocktower:fix-266 branch Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment