Skip to content

No legacy #39

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

Merged
merged 5 commits into from
Dec 6, 2019
Merged

No legacy #39

merged 5 commits into from
Dec 6, 2019

Conversation

odolbeau
Copy link
Member

@odolbeau odolbeau commented Dec 5, 2019

This is a rebase of #32
It removes a lot of classes, it will be way easier to run php-cs-fixer & PHPStan! ^^

(Need a major release)

@odolbeau odolbeau requested a review from Nyholm December 5, 2019 10:47
@Nyholm
Copy link
Member

Nyholm commented Dec 5, 2019

Oh. Interesting. I was just it would be a BC break. I’ll review this carefully shortly.

@rvanlaak
Copy link
Member

rvanlaak commented Dec 5, 2019

We can use PHPStan's baseline feature to continue, so no worries about a dependency on that 👍

https://medium.com/@ondrejmirtes/phpstans-baseline-feature-lets-you-hold-new-code-to-a-higher-standard-e77d815a5dff

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking. I think there are BC breaks.

I like that you rebased this and I do think it makes sense to let 1.1.0 (or 1.1.1) be the last 1.x release and then release 2.0 with this change.

It would make this library sooo much simpler. =)

*
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
class SymfonyPort
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is removing this class a BC break?

@@ -58,18 +58,8 @@ final class FileStorage implements Storage, TransferableStorage
* @param array $dir
* @param array $options
*/
public function __construct($writer, $reader, array $dir, array $options = [])
public function __construct(TranslationWriterInterface $writer, TranslationReaderInterface $reader, array $dir, array $options = [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the possibility to use TranslationLoader here is a BC break Im afraid.

@rvanlaak rvanlaak mentioned this pull request Dec 5, 2019
@odolbeau
Copy link
Member Author

odolbeau commented Dec 6, 2019

I merge this PR.
Before releasing a v2 of this storage, let's wait for php-translation/common v2 to be released (to be done once php-translation/common#24 is merged).

TODO before v2:

@odolbeau odolbeau merged commit db13bb8 into php-translation:master Dec 6, 2019
@odolbeau odolbeau deleted the no-legacy branch December 6, 2019 13:26
@odolbeau odolbeau mentioned this pull request Dec 6, 2019
@Nyholm
Copy link
Member

Nyholm commented Dec 6, 2019

Wohoo!

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.

3 participants