Skip to content

Reg exp patter is optionlity#88

Open
brilliant-almazov wants to merge 2 commits intot1gor:masterfrom
brilliant-almazov:reg-exp-patter-is-optionlity
Open

Reg exp patter is optionlity#88
brilliant-almazov wants to merge 2 commits intot1gor:masterfrom
brilliant-almazov:reg-exp-patter-is-optionlity

Conversation

@brilliant-almazov
Copy link

No description provided.

Copy link
Owner

@t1gor t1gor left a comment

Choose a reason for hiding this comment

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

Please see comments on code.

* @param string $encoding - encoding
*/
public function __construct($content, $encoding = self::DEFAULT_ENCODING)
public function __construct($content, $encoding = self::DEFAULT_ENCODING, array $regExpPatternReplace = array())
Copy link
Owner

Choose a reason for hiding this comment

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

If you would like to pass in your own replaces, I guess it would make sense to merge our expected replaces with the user-ones, or at least pass the default ones by default:

Suggested change
public function __construct($content, $encoding = self::DEFAULT_ENCODING, array $regExpPatternReplace = array())
public function __construct($content, $encoding = self::DEFAULT_ENCODING, array $regExpPatternReplace = array('@' => '\@'))

$this->prepareRules();

$this->regExpPatternReplace = empty($regExpPatternReplace)
? array('@' => '\@')
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure you would want to replace it but not merge? Could you please provide a test for this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants