Page MenuHomePhabricator

scm-prompt: standardize testing style and formatting
ClosedPublic

Authored by ryanmce on Sep 22 2017, 10:37 AM.

Details

Reviewers
stash
Group Reviewers
Restricted Project
Commits
rFBHGX7d5162acb093: scm-prompt: standardize testing style and formatting
Test Plan

rt

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ryanmce created this revision.Sep 22 2017, 10:37 AM
stash accepted this revision.Sep 22 2017, 10:45 AM
stash added a subscriber: stash.
stash added inline comments.
scripts/scm-prompt.sh
64

Why do we prefer builtin here? Can you also add it in the big comment above, that we should use "builtin" for echo and "command" for everything else

101

Same question as in previous diff - are there any practical implications, or it's just a style thing?

This revision is now accepted and ready to land.Sep 22 2017, 10:45 AM
This revision was automatically updated to reflect the committed changes.
ryanmce added inline comments.Sep 22 2017, 11:57 AM
scripts/scm-prompt.sh
64

Whoops I missed this comment. I'll add a follow-up.

I changed to builtin because it works better in zsh -- in fact command cd doesn't work in zsh at all for my purposes, so I just tried to be consistent and use builtin for things I knew were builtin to the shells we support.

101

It's important if there are spaces or glob characters like *

mqian added a subscriber: mqian.Sep 26 2017, 5:19 PM
mqian added inline comments.
scripts/scm-prompt.sh
193

Adding the outer double quotes without removing the inner quotes causes literal quotes to appear in the prompt.

I'm not sure how my testing missed this; occur a task last night to fix it and will get it in today.

ryanmce added inline comments.Sep 27 2017, 5:12 AM
scripts/scm-prompt.sh
193

Fixed in D823