Skip to content
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

Added TimeInterval and ParserSettings tests #25

Merged
merged 1 commit into from
Nov 2, 2016

Conversation

PeeHaa
Copy link
Collaborator

@PeeHaa PeeHaa commented Oct 30, 2016

This PR is related to #6

These tests are failing because of some logic error in the ParserSettings class:

  • testGetWordSeparatorWhenUsingTheDefault will fail on default value because getWordSeparatorWhenUsingTheDefault will return null instead of the expect string
  • testGetWordSeparatorExpressionWhenUsingTheDefault will fail on default value because getWordSeparatorWhenUsingTheDefault will return null instead of the expect string
  • Not sure exactly why testGetWordSeparatorExpressionWhenManuallySet fails. Might be my fault.

General remarks:

  • I am not sure what use getSeparationType has
  • Currently the replacement methods ParserSettings::*Expression() will break when the replacement text contains valid regex. Maybe preg_quote() the characters? This still needs tests.
  • Some parameters / method names are confusing when looking at just this class. E.g. I have no idea what $multipleSeparationType is / means.
  • Remove temporary variables in ParserSettings::*Expression() and just return the new expression

@ekinhbayar
Copy link
Owner

ekinhbayar commented Oct 31, 2016

@PeeHaa I'm reviewing these at the moment, just a random thought related to string|null values, I wonder would it be better for us to use '' instead of null like:

$trailing = $parts['trailing'] ?? '';
$interval = $parts['interval'] ?? '';

Though when leading/trailing is not needed they're null:
new TimeInterval($interval, $intervalOffset, $intervalLength, null, $trailing);

Actually nevermind, I feel this was a stupid thought anyway, but keeping this bit of the comment JIC you'd like to say something.

Another thing, what do you think of having a single string $multipleSeparationString = ',' instead of separating word/symbol? This would also allow us to get rid of the getSeparationType and the two constants which are only used in relation to that. see

The reasoning behind having two separate types was to somehow allow the use of words as well as symbols. Initially and as of the current master I had two regexes for both separator types but looking at it now I see the word regex is enough for both anyway.

So, what do you think of below:

class ParserSettings
{
    private $leadingSeparationString;
    private $keepLeadingSeparator;
    private $multipleSeparationString;

    # Leading separator with capturing groups
    public static $leadingGroupSeparator = "/(.*)\s+(?:in)\s+(.*)/ui";
    public static $stringSeparator = "/^(?<first>.*?)\s?,\s?(?<next>.*)$/ui";

    public function __construct(
        string $leadingSeparationString = 'in',
        bool $keepLeadingSeparator = false,
        string $multipleSeparationString = ','
    )
    {
        $this->leadingSeparationString = $leadingSeparationString;
        $this->keepLeadingSeparator = $keepLeadingSeparator;
        $this->multipleSeparationString = $multipleSeparationString;
    }

    public function getLeadingSeparator() : string
    {
        return $this->leadingSeparationString;
    }

    public function getStringSeparator() : string
    {
        return $this->multipleSeparationString;
    }

    public function getLeadingSeparatorExpression() : string
    {
        return preg_replace("/in/", $this->getLeadingSeparator(), self::$leadingGroupSeparator);
    }

    public function getStringSeparatorExpression() : string
    {
        return preg_replace("/,/", $this->getStringSeparator(), self::$stringSeparator);
    }

    public function keepLeadingSeparator() : bool
    {
        return $this->keepLeadingSeparator;
    }

}

But obviously this is not enough for allowing interchangeable use of a word and a symbol as a separator using the same parser instance, thoughts?

@PeeHaa
Copy link
Collaborator Author

PeeHaa commented Nov 2, 2016

I think this can be merged. because the test that fail do so because of actual issues in the code. So the failing tests should be solved by fixing the code.

Another thing, what do you think of having a single string $multipleSeparationString = ',' instead of separating word/symbol?

Yes very much agree.

So, what do you think of below:

Already much nicer imo. But yes that will be a problem for allowing interchangeable use of a word and a symbol as a separator. I still like the idea of some collection in whatever for to do this.

BTW I think the general remarks should be moved to a new issue.

@ekinhbayar
Copy link
Owner

Agreed on all above. Then I'll merge this PR and we can continue improving from here onwards.

@ekinhbayar ekinhbayar merged commit 0cf215d into ekinhbayar:master Nov 2, 2016
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