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

Fix attempt for Google and "All day". #192

Merged
merged 5 commits into from
Feb 19, 2024
Merged

Conversation

vever001
Copy link
Contributor

Context and Purposes

Follow up of #184 and comment #184 (comment)

Copy link
Collaborator

@alies-dev alies-dev left a comment

Choose a reason for hiding this comment

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

that's a great fix! Thank you a lot for your work on this, @vever001!

I have one minor note, please take a look.

@@ -35,4 +35,12 @@ public function it_correctly_generates_all_day_events_by_dates(): void
$this->generator()->generate($this->createEventMultipleDaysViaStartEndWithTimezoneLink())
);
}

/** @test */
public function it_correctly_generates_all_day_events_by_dates_diff_tz(): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think having this test is beneficial for all generators. How about moving it to the GeneratorTestContract?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed!
Maybe also moving the ones I added as well?

}

$from = (clone $fromDate)->modify('midnight');
$from = (clone $fromDate);
Copy link
Contributor

Choose a reason for hiding this comment

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

With my old code, we're removing the hours/days portion. I guess this probably wasn't required and it only was done for non UTC dates.

Copy link
Contributor

@atymic atymic left a comment

Choose a reason for hiding this comment

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

Awesome to see all the activity here in this repo :)
Good job all!

@atymic
Copy link
Contributor

atymic commented Jan 30, 2024

Failing test looks fine as well, just needs a snapshot update

@vever001
Copy link
Contributor Author

Thanks for the feedback.
I need to do some more testing, when I have time. Some things still don't quite add up.
With all day, some services seem to be exclusive on the end date (e.g.: Google) while others seem to be inclusive (e.g.: Yahoo). This is not straightforward...

@atymic
Copy link
Contributor

atymic commented Jan 30, 2024

Yea, it's a shame we can't test them again the provider itself.

@alies-dev
Copy link
Collaborator

Thanks @vever001 and @atymic
I'm happy to see your activity here. I think the PR is almost done; even at this stage it's still an improvement. But it's still cab be improved. I'm ok to merge it as is and release and wait for a new PR to improve it even more. But I would also love if you make these improvements within this PR. That's up to you, I'm ok with both options, just want to keep the tempo and make this awesome job done :)

@samuelreichor
Copy link

Hi guys, I'm looking forward for the merge :)

@alies-dev
Copy link
Collaborator

Hey @vever001!

Is it ready to be reviewed?

@vever001
Copy link
Contributor Author

Hi @alies-dev, sure please do :)
I just didn't have time yet to improve tests, comments #192 (comment) and #192 (comment)
But any help is welcome.

--
I have some ideas on how to improve this lib but this would clearly be out of scope for this issue and introduce breaking changes. See for example this vever001@39a9bbd
Also some kind of plugin system would be nice (with class discovery, plugins using PHP 8 class attributes?) but I don't know if the requirement here are to not have any dependencies (e.g: symfony or other).

@alies-dev alies-dev self-requested a review February 19, 2024 05:57
Copy link
Collaborator

@alies-dev alies-dev left a comment

Choose a reason for hiding this comment

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

@vever001
Great! It can be improved even more, but I believe the PR in it's current shape is still an improvement and thus we can merge it! Thank you for your amazing work!

@alies-dev alies-dev merged commit 176efde into spatie:master Feb 19, 2024
6 checks passed
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.

4 participants