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

Make xls spreadsheet included via composer #137

Merged
merged 6 commits into from
Dec 12, 2022
Merged

Make xls spreadsheet included via composer #137

merged 6 commits into from
Dec 12, 2022

Conversation

copperhead57
Copy link
Collaborator

xls.php - include vendor/autoload.php
xls.php - left align plot
xls.php - add test if imdbID field is blank to stop corrupt xls file
amend config.sample.php

xls.php - include vendor/autoload.php
xls.php - left align plot 
xls.php - add test if imdbID field is blank to stop corrupt xls file
amend config.sample.php
@johanneskonst
Copy link
Collaborator

As per #153, i'm all for adding external libraries to composer but not too happy with including all their source in videodb's git repo. if you don't mind i'd like to workout #153 first before (partially) merging this.

@johanneskonst johanneskonst added the onhold Stuff that's not needed right now, or is awaiting other prerequisites... label Dec 8, 2022
…ample.php (thus config.inc.php).

this means that if user customizes the contains of data to spreadsheet, it will not be lost when refreshing version of videodb.
@copperhead57 copperhead57 self-assigned this Dec 12, 2022
@johanneskonst
Copy link
Collaborator

@copperhead57 could you remove all changes in the vendor folder from this pull request? so that i can merge this with only the changes from composer.json, config.sample, core/xls and xls.inc (and after that merge all remaining PRs)

And what are your thoughts on merging all configuration options from xls.inc.php (and pdf.inc.php also) into the config.sample? I think that any 'performanceboost' from putting unused options in a separate file aren't even measurable these days, and it makes it predictable on where to find configuration options that might need changes...

@copperhead57
Copy link
Collaborator Author

@johanneskonst I am not exactly sure how to do vendor folder changes manually as I am only novice user of composer (my IDE tool does all the work).
As all the other PR's are independent of composer lib changes can they go ahead anyway?

As for merging config into config.sample a good idea but I have found out that it causes a problem if the user doesn't update their config.inc file. Without some method to auto update their file i foresee issues being raised. this is not a show stopper just an observation, I have done a quick change to this piece of work with a work around.

@johanneskonst
Copy link
Collaborator

Ah, i'll cleanup vendor later then, i just want to clear the list of PR and issues so we can move forward.
And handling the missing config options is no problem, i'll cover that lateron...

@johanneskonst johanneskonst merged commit 3e73f37 into andig:master Dec 12, 2022
@copperhead57 copperhead57 deleted the spreadsheetwriter-via-composer branch December 13, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
onhold Stuff that's not needed right now, or is awaiting other prerequisites...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants