-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
[Translation][File dumper] allow get file content without writing in file. #15786
Conversation
aitboudad
commented
Sep 14, 2015
Q | A |
---|---|
Bug fix? | no |
New feature? | yes |
BC breaks? | no |
Deprecations? | no |
Fixed tickets | #14881 |
Tests pass? | yes |
License | MIT |
@@ -63,26 +63,34 @@ public function setBackup($backup) | |||
*/ | |||
public function dump(MessageCatalogue $messages, $options = array()) | |||
{ | |||
if (!array_key_exists('path', $options)) { | |||
throw new \InvalidArgumentException('The file dumper needs a path option.'); | |||
if (!array_key_exists('path', $options) && !array_key_exists('format_callback', $options)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name format_callback looks weird to me. This callback is not responsible for any formatting, but for saving
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's about save_handler
?
8969df2
to
6d2666e
Compare
* added options `as_tree`, `inline` to YamlFileDumper | ||
* added option `save_handler` to FileDumper. | ||
* added option `json_encoding` to JsonFileDumper. | ||
* added options `as_tree`, `inline` to YamlFileDumper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The three dots should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
6d2666e
to
0ed46bb
Compare
0ed46bb
to
e116cb8
Compare
ping @symfony/deciders |
This looks like a workaround for a bad architecture. It doesn't make sense that the formating is tied to dumping to files at all. We could alternatively refactor out the formatting part of the FileDumpers. So basically refactor the |
e116cb8
to
6c3826c
Compare
@Tobion thanks for pointing me in the right direction. Seems making |
$this->assertEquals(file_get_contents(__DIR__.'/../fixtures/resources.ini'), file_get_contents($tempDir.'/messages.en.ini')); | ||
|
||
unlink($tempDir.'/messages.en.ini'); | ||
$this->assertEquals(file_get_contents(__DIR__.'/../fixtures/resources.ini'), $dumper->formatCatalogue($catalogue, 'messages')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertStringEqualsFile
?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, didn't know it :)
6c3826c
to
3cbba5e
Compare
I see |
yes I already removed the |
It don't like that formatting and dumping is still in one class. But this way it might be good enough 👍 |
3cbba5e
to
805acc9
Compare
Thank you @aitboudad. |
…out writing in file. (aitboudad) This PR was merged into the 2.8 branch. Discussion ---------- [Translation][File dumper] allow get file content without writing in file. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Fixed tickets | #14881 | Tests pass? | yes | License | MIT Commits ------- 805acc9 fixed typo. 9b877cf [Translation][File dumper] allow get file content without writing in file.