Skip to content
This repository was archived by the owner on Sep 20, 2021. It is now read-only.

Fix generate space (\s) character. #20

Closed
wants to merge 1 commit into from
Closed

Fix generate space (\s) character. #20

wants to merge 1 commit into from

Conversation

Metalaka
Copy link
Member

\s token was previously considered like \h.
Now \s will be interpreted as \h or \v.

@Hywan Hywan self-assigned this Aug 12, 2015
@Hywan Hywan added the ready label Aug 12, 2015
@Hywan
Copy link
Member

Hywan commented Aug 12, 2015

@Metalaka I don't understand where the bug was. We already has this choice for s to be interpreted as h or v in the switch. Not there is no break, this is the trick.

However, I understand this is error prone and then your patch is welcomed. Is it why you had in mind?

@Hywan Hywan added in progress and removed ready labels Aug 12, 2015
@Metalaka
Copy link
Member Author

Please look result of http://3v4l.org/DsOA7

The previous line $value = $this->_sampler->getInteger(0, 1) ? 'h' : 'v'; is totally useless since there is no break,
so the following case is directly executed and the choice (h or v) is not used.

@Hywan
Copy link
Member

Hywan commented Aug 12, 2015

Ok :-].

@Hywan
Copy link
Member

Hywan commented Aug 12, 2015

Can you please reword your commit message based on http://hoa-project.net/Literature/Contributor/Guide.html#Commiting_a_patch:

It must answer to the following questions:

  1. What was the issue?
  2. How to address this issue?
  3. How did we address the issue?

Thanks!

When generating a value for `\s` only value of `\h` was used.
We must choose a value (`\h` or `\v`) for replace `\s` and then generate a
valid value for our remplacement.
@Metalaka
Copy link
Member Author

Done.

@Hywan
Copy link
Member

Hywan commented Aug 13, 2015

Excellent, thanks!

@Hywan
Copy link
Member

Hywan commented Aug 13, 2015

Merged in ad0d526.

@Hywan Hywan closed this Aug 13, 2015
@Hywan Hywan removed the in progress label Aug 13, 2015
@Metalaka Metalaka deleted the fix/spaceRandom branch August 13, 2015 11:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

2 participants