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

Change artisan command and prefix #15

Closed
Lednerb opened this issue Jun 14, 2022 · 2 comments · Fixed by #16
Closed

Change artisan command and prefix #15

Lednerb opened this issue Jun 14, 2022 · 2 comments · Fixed by #16
Labels
enhancement New feature or request implemented

Comments

@Lednerb
Copy link
Contributor

Lednerb commented Jun 14, 2022

Hi @wdelfuego and thanks for this great package.

I'd like to request the following change:

To create the default calendar data provider there is the following artisan command provided by this package:

php artisan create:default-calendar-data-provider

The command should be refactored to the following usage:

php artisan nova-calendar:create-default-calendar-data-provider

This will ensure that you can add more commands in the future under the same namespace and will separate the CLI from default artisan commands to those that are supported by third parties.

If you agree with this change I will submit the needed changes as a PR in the next days.

@wdelfuego
Copy link
Owner

HI @Lednerb, good point, thanks for your input!

I agree to the change and will accept the PR.

Maybe nova-calendar:create-default-data-provider would be even better since the nova-calendar prefix already indicates what it's for and I don't expect the calendar to have any non-calendar data providers anytime soon ;)

I know you're aware of this because of your thumbs up on my comment in the PHP7 support PR but for completeness' sake, please check that you agree to your changes being distributed under this package's dual licensing model.

Related; I'm playing with the idea of changing the naming from dataprovider to datasource since providers in Laravel apps are generally service providers and not data providers, if you have any thoughts on that naming / architectural issue I'd be happy to hear them, too.

Thanks for helping!

@Lednerb
Copy link
Contributor Author

Lednerb commented Jun 14, 2022

I know you're aware of this because of your thumbs up on my comment in the PHP7 support PR but for completeness' sake, please check that you agree to your changes being distributed under this package's dual licensing model.

I'm absolutely fine with it. Btw I like your dual licensing model a lot and consider to use it maybe in the future for my projects as well. (Also btw.: Maybe it's nice to link the GNU License in the README.md with https://choosealicense.com/licenses/agpl-3.0/ for an easy overview)


Related; I'm playing with the idea of changing the naming from dataprovider to datasource since providers in Laravel apps are generally service providers and not data providers, if you have any thoughts on that naming / architectural issue I'd be happy to hear them, too.

Actually I like the DataProvider naming convention because it feels like the laravel way of naming it, altough most of the times ServiceProviders are used. But I don't think that it is misleading or a naming issue in this case.

I associate a DataSource to different things like external databases, queues, local directories or external APIs and that's not the case here.

Within the CalendarDataProvider.php I have to provide the data for the calendar tool.

It's a good name, let's stick with it! ;-)

Lednerb added a commit to Lednerb/nova-calendar that referenced this issue Jun 14, 2022
@wdelfuego wdelfuego added enhancement New feature or request implemented labels Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants