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 csv options configurable via $options #145

Merged
merged 2 commits into from
May 9, 2017

Conversation

Zae
Copy link
Contributor

@Zae Zae commented May 9, 2017

Make csv options configurable via $options, defaults are set to the PHP defaults.

Translations::fromCsvFile($destination, ['delimiter' => ';']);

$translations->toCsvFile(
    $pathinfo['dirname'] . '/' . $pathinfo['filename'] . '.csv',
    [
        'includeHeaders' => true,
        'delimiter' => ';'
    ]
);

@Zae
Copy link
Contributor Author

Zae commented May 9, 2017

I see the tests fail because of incompatibilty with the parameter count in older versions of PHP, any suggestions to fix this cleanly?

@oscarotero
Copy link
Member

A way to make it compatible with php 5.5 is using version_compare and execute the function without the last argument if the php version is lower than 5.5.4

@Zae
Copy link
Contributor Author

Zae commented May 9, 2017

Yes, I was thinking about that, but thought that might be weird that some options won't be available and the code might get a little sloppy with version_compare's all over the place.

I'll have a little think about it and update the PR later :)

@Zae Zae force-pushed the feature/csv_options branch from c22e196 to c125cc4 Compare May 9, 2017 15:57
@Zae
Copy link
Contributor Author

Zae commented May 9, 2017

I added a function that wraps the csv functions with backward compatible versions.

@oscarotero oscarotero merged commit 9454424 into php-gettext:master May 9, 2017
@oscarotero
Copy link
Member

Thank you 👍

@oscarotero
Copy link
Member

Ok, I've changed a bit your code. The same options have been extended to csvDictionary (extractor and generator) and, althought there's support for the escape_char argument in fgetcsv since php 5.3, I think is better to use the same version than fputcsv, in order to maintain the same support in csv generation and extraction.

@Zae
Copy link
Contributor Author

Zae commented May 10, 2017

Sounds reasonable, I wasn't using the CsvDictionary so didn't think about adding it there, good that you did :)

When do you think you will tag a new release?

@oscarotero
Copy link
Member

v4.4.0 released

@Zae Zae deleted the feature/csv_options branch May 18, 2017 20:22
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.

2 participants