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

Sort staged and unstaged files by path. #1671

Merged
merged 4 commits into from Aug 29, 2018

Conversation

Projects
None yet
3 participants
@phyllisstein
Contributor

phyllisstein commented Aug 29, 2018

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

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

Previously, staged and unstaged files returned from Git were sorted alphabetically by path in the "Staged Changes" pane, but grouped by change status in the "Unstaged Changes" pane:

before

This made navigating the two panes counterintuitive, as the rhyme and reason behind the groupings in "Unstaged" was not immediately obvious and the sort pattern changed midway through a typical Git workflow.

This small change simply drops a sort() call into Repository#getUnstagedChanges and Repository#getStagedChanges. With this fix, both panes show files sorted by pathname:

after

Alternate Designs

I considered diving a little deeper into the Git API and exploring why the two results were sorted so differently, but settled on this as a comfortable middle ground between actually shipping a change and extensive Talmudic study of the code base.

Benefits

For my money, at least, this is a fast and easy way of ironing out a frustrating UX tic.

Possible Drawbacks

Folks may have become accustomed to the bad behavior, but otherwise, I can't see any issues.

Applicable Issues

#1278

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Aug 29, 2018

Member

I'm 👍 on accepting this if you can get the tests green 😄 It looks like the only failures are from assertions that we're currently making about unstaged changes in the other order.

Member

smashwilson commented Aug 29, 2018

I'm 👍 on accepting this if you can get the tests green 😄 It looks like the only failures are from assertions that we're currently making about unstaged changes in the other order.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 29, 2018

Coverage Status

Coverage increased (+0.05%) to 80.215% when pulling ce32420 on phyllisstein:master into 7f89ab7 on atom:master.

coveralls commented Aug 29, 2018

Coverage Status

Coverage increased (+0.05%) to 80.215% when pulling ce32420 on phyllisstein:master into 7f89ab7 on atom:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 29, 2018

Coverage Status

Coverage increased (+0.06%) to 80.224% when pulling 093a2fc on phyllisstein:master into 7f89ab7 on atom:master.

coveralls commented Aug 29, 2018

Coverage Status

Coverage increased (+0.06%) to 80.224% when pulling 093a2fc on phyllisstein:master into 7f89ab7 on atom:master.

@phyllisstein

This comment has been minimized.

Show comment
Hide comment
@phyllisstein

phyllisstein Aug 29, 2018

Contributor

D'oh, I always forget that paths look different on Windows 🤕🌴. Should be good to go now---thanks for your feedback! There's a flapping test in AppVeyor, though, and I'm not sure how to trigger a new build.

Contributor

phyllisstein commented Aug 29, 2018

D'oh, I always forget that paths look different on Windows 🤕🌴. Should be good to go now---thanks for your feedback! There's a flapping test in AppVeyor, though, and I'm not sure how to trigger a new build.

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Aug 29, 2018

Member

Build is retriggered and green. Thanks! 🙇

Member

smashwilson commented Aug 29, 2018

Build is retriggered and green. Thanks! 🙇

@smashwilson smashwilson merged commit 5cefdf0 into atom:master Aug 29, 2018

6 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: stable Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 80.215%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment