Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Fix spaces-to-tabs and tabs-to-spaces to use "tab stops" correctly #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

giseburt
Copy link

@giseburt giseburt commented May 2, 2015

With these corrections, the tabs-to-spaces and spaces-to-tabs conversions match the layout of the core editor display, as well as are capable of going round-trip (spaces -> tabs -> spaces) without losing the visual positions of the non-whitespace characters.

I also had to beef up the specs a little, since they would fail on non-Travis (IOW, machines that had Atom installed and had some personalization) machines. It is just basic hygiene of setting parameters back to expected values before starting the next test. (More could probably be done, but what I have now appears to be fairly robust.)

I changed the specs to reflect the new expected behavior.

No documentation changes are necessary.

Animation of the corrected behavior:
after

@giseburt
Copy link
Author

giseburt commented May 3, 2015

Btw, this should now match the behavior of the equivalent commands in TextMate.

@izuzak
Copy link
Contributor

izuzak commented Jul 30, 2015

Sorry for the delay in reviewing this, @giseburt.

@nathansobo You reviewed and worked on similar PRs in Atom core, I think (atom/atom#5658, atom/atom#7563, atom/atom#7602) -- when you get a chance, could you take a look at this one as well? 🙏

Also, I noticed that the PR no longer merges without conflicts. Probably minor things to resolve compared to master -- could you update the PR so that it is mergeable, @giseburt?

@nathansobo
Copy link
Contributor

I'm confused about the changes to the specs regarding the various settings... many of them seem to be testing the config system rather than anything particular to this package. It would be easier to review this if it focused exclusively on tab to whitespace conversion.

waitsForPromise ->
atom.packages.activatePackage("whitespace")

it 'should start with the package active', ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why you added this...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants