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 configuration for location of strings in Scanner #256

Closed
wants to merge 2 commits into from

Conversation

Arzaroth
Copy link

This addresses both #208 and #143

I didn't provide tests for this PR since there is no concrete implementation of CodeScanner in this repository

@oscarotero
Copy link
Member

Hi, thanks for your work with these issues!

I'd rather have these two issues in two different pull requests, because I have some doubts:

  • I'm agree with adding the option to enable or disable the translations locations, but I don't like the option name because it's confusing. I'd rename to addReferences(), to keep consistency with the $translations->getReferences() function.
  • I'm not sure about the relative path, and this is why I'd like to have a different pull request. The reason is that perhaps it add too much responsabilities to a class that is created to scan an unique file. If anyone want to use a different file path, the solution is simple: passing directly the content and filename. For example:
$contents = file_get_contents($file);
$relativePath = get_the_relative_path($file);

$translations = $scanner->scanString($contents, $relativePath);

This function is already present in the interface (https://github.com/php-gettext/Gettext/blob/master/src/Scanner/ScannerInterface.php#L21) and this way is much more flexible, so I'd wouldn't add this feature (at least, for now).

@Arzaroth
Copy link
Author

Points taken.

I'll close this PR and submit two new ones.

Regarding relative paths, I didn't realize what you suggested was possible. It is indeed a nice workaround and I understand your argument on this matter.

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