Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add one more "unicode" option in expand_env_variable—I have a bullet in my prompt that was blowing it up #102

Merged
merged 3 commits into from Jun 24, 2013

Conversation

Projects
None yet
3 participants
Contributor

pragdave commented Jun 22, 2013

...my prompt that was blowing it up

Contributor

tuncer commented Jun 22, 2013

Thanks @pragdave, here's a couple suggestions:

  • move the ReOpts up and use it in both calls to re:replace/4. this will also avoid the overlong line.
  • fix the commit message as outlined here

Otherwise this looks reasonable to me.

Contributor

pragdave commented Jun 23, 2013

I changed the commit msg, but the two sets of options are different, so I
didn't want to merge them. Instead, I extracted what was in common and used
that.

On Sat, Jun 22, 2013 at 4:16 AM, Tuncer Ayaz notifications@github.comwrote:

Thanks @pragdave https://github.com/pragdave, here's a couple
suggestions:

  • move the ReOpts up and use it in both calls to re:replace/4. this
    will also avoid the overlong line.
  • fix the commit message as outlined herehttps://github.com/rebar/rebar/blob/master/README.md#writing-commit-messages

Otherwise this looks reasonable to me.


Reply to this email directly or view it on GitHubhttps://github.com/rebar/rebar/pull/102#issuecomment-19853791
.

Contributor

tuncer commented Jun 23, 2013

I changed the commit msg, but the two sets of options are different, so I didn't want to merge them. Instead, I extracted what was in common and used that.

Thanks. While the extra {return, list} was previously only used for the final re:replace, it works correctly for both calls. So, to simplify the code and avoid the now overlong line 199, I suggest to make ReOpts [global, {return, list}, unicode].

Contributor

tuncer commented Jun 24, 2013

Thanks. The change looks good to me now, but you seem to have mistakenly forgotten the 'a' in 'crash' :).

Contributor

pragdave commented Jun 24, 2013

I can live with that
On Jun 24, 2013 4:50 AM, "Tuncer Ayaz" notifications@github.com wrote:

Thanks. The change looks good to me now, but you seem to have mistakenly
forgotten the 'a' in 'crash' :).


Reply to this email directly or view it on GitHubhttps://github.com/rebar/rebar/pull/102#issuecomment-19895504
.

@dizzyd dizzyd added a commit that referenced this pull request Jun 24, 2013

@dizzyd dizzyd Merge pull request #102 from pragdave/master
Add one more "unicode" option in expand_env_variable—I have a bullet in my prompt that was blowing it up
be8b022

@dizzyd dizzyd merged commit be8b022 into rebar:master Jun 24, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment