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

Add DateProcessor #73

Merged
merged 2 commits into from
Oct 27, 2019
Merged

Add DateProcessor #73

merged 2 commits into from
Oct 27, 2019

Conversation

LeJeanbono
Copy link
Contributor

Fix #28


class DateProcessor
{
use TagCoverageTrait;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this trait is required here, because this processor won't change any tag, but rather create a new one.

BTW we could use TagSearchTrait to make sure we find the tag we depend on.

$day = null;
$month = null;
$year = null;
foreach ($covered as $tag) {
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need this loop IMO

}
}
$entry['month'] = $day . '~' . $month;
$entry['_date'] = new DateTime(date('d/m/Y', strtotime($month . ' ' . $day . ' ' . $year)));
Copy link
Owner

Choose a reason for hiding this comment

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

  • What do you think if we make the _date value configurable via constructor? and keep defaults to _date
  • I would prefer it to be DateTimeImmutable

{
$processor = new DateProcessor();
$entry = $processor([
'month' => '"1~" # jan',
Copy link
Owner

Choose a reason for hiding this comment

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

It should test against 1~jan, because of this listener's behaviour

Copy link
Owner

Choose a reason for hiding this comment

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

We need to check it against 1~jan

{
$processor = new DateProcessor();
$entry = $processor([
'month' => 'Jul # "~4"',
Copy link
Owner

Choose a reason for hiding this comment

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

as mentioned before, it should test against Jul~4

Copy link
Owner

Choose a reason for hiding this comment

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

we need to check it against Jul~4

@renanbr
Copy link
Owner

renanbr commented Oct 18, 2019

Thanks for this PR,

What do you think it should be the behaviour for these scenarios?

['month' => 'Jul', 'year' => '2000']; // missing day in month
['month' => '05', 'year'=> '2000'];   // invalid month value
['month' => '1~jan', 'year' => '98']; // invalid year
['month' => 'foo', 'year' => 'bar'];  // invalid
['month' => '1~jan'];                 // missing year
['year' => '2000'];                   // missing month
[];                                   // missing everything 
['_date' => 'I do exist'];            // already existing _date

a) exception
b) _date to null
c) do nothing
d) something else?

I tend to say c, WDYT?


class DateProcessor
{
use TagCoverageTrait;
Copy link
Owner

Choose a reason for hiding this comment

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

Replace this trait by TagSearchTrait, then we can call tagSearch() to get the real tag name for year and month

$month = null;
$year = null;
foreach ($covered as $tag) {
if ($tag === 'month') {
Copy link
Owner

Choose a reason for hiding this comment

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

TagSearchTrait->tagSearch() could be useful here

{
$processor = new DateProcessor();
$entry = $processor([
'month' => '"1~" # jan',
Copy link
Owner

Choose a reason for hiding this comment

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

We need to check it against 1~jan

{
$processor = new DateProcessor();
$entry = $processor([
'month' => 'Jul # "~4"',
Copy link
Owner

Choose a reason for hiding this comment

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

we need to check it against Jul~4

tests/Processor/DateProcessorTest.php Outdated Show resolved Hide resolved
tests/Processor/DateProcessorTest.php Show resolved Hide resolved
@renanbr
Copy link
Owner

renanbr commented Oct 21, 2019

Overall I'm happy 😄 I left some comments before getting this merged

@codecov
Copy link

codecov bot commented Oct 22, 2019

Codecov Report

Merging #73 into master will increase coverage by 0.29%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #73      +/-   ##
============================================
+ Coverage     90.38%   90.68%   +0.29%     
- Complexity      175      180       +5     
============================================
  Files             9       10       +1     
  Lines           468      483      +15     
============================================
+ Hits            423      438      +15     
  Misses           45       45
Impacted Files Coverage Δ Complexity Δ
src/Processor/DateProcessor.php 100% <100%> (ø) 5 <5> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1922ca...ee3d398. Read the comment docs.

@renanbr renanbr merged commit ee3d398 into renanbr:master Oct 27, 2019
@renanbr
Copy link
Owner

renanbr commented Oct 27, 2019

thank you @LeJeanbono !
merged as 3bcd9b6

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.

implement DateProcessor
2 participants