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 Custom Resource Events and Permissions #13

Conversation

ali-raza-saleem
Copy link

@ali-raza-saleem ali-raza-saleem commented Jun 8, 2022

On behalf of @muhammadsaeedparacha

Discussions: #11 and #12

Copy link
Owner

@wdelfuego wdelfuego left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request!

While I agree that it'd be nice to be able to extract multiple calendar events from a single Nova resource, this implementation does not meet my coding standards or architectural vision for the project so I won't be accepting this pull request.

To give you some examples;

  • resourceToEvent nor resourceToEvents should be public methods
  • I don't think resourceToEvent's argument $dateAttribute should be a DateTimeInterface, but if it should, it should most definitely no longer be called $dateAttribute
  • having resourceToEvent and resourceToEvents with slightly different method signatures is confusing
  • $novaResourceClass::toEvents($this) is a call to a static method. Do you really want developers to implement their custom toEvents behavior statically? I'd vote for a normal instance method that returns the calendar event(s) for a single Nova resource instance and I'd go for an implementation that doesn't require passing the data provider to the Nova resource.
  • method_exists($novaResourceClass, 'toEvents') is not a safe way to implement that feature, I'd want to work with an interface and possibly a trait to make life easier for the developer
  • I prefer the resourceToEvent method to use the Event::fromResource factory method over calling the constructor directly
  • Line 187 contains a bug

These are just examples and pointers to help you understand my decision. These are not todo's that I want you to address in order to accept your pull request. What I will do is think about my own implementation of this feature because I see value in having it.

I hope this implementation and my feedback on it is valuable for you in your own projects.

Thanks for contributing!

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.

None yet

2 participants