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

Change interpret() return value to conform with Layout\ReaderInterface #908

Merged
merged 1 commit into from
Jan 8, 2015
Merged

Change interpret() return value to conform with Layout\ReaderInterface #908

merged 1 commit into from
Jan 8, 2015

Conversation

Vinai
Copy link
Contributor

@Vinai Vinai commented Jan 5, 2015

This PR to the develop branch replaces #754.

The only change is the return type of the interpret() method so it conforms to the interface declaration.

@Vinai
Copy link
Contributor Author

Vinai commented Jan 5, 2015

Test failure is because of the issue fixed in #913, not because of this PR.

Change View\Layout\Reader\Remove::interpret() return value to conform
with Layout\ReaderInterface::interpret()
@vpelipenko
Copy link
Contributor

@Vinai, for the future if you want to deliver to Magento source code some similar changes it will be better to combine them in one PR instead of creating a bunch of different PRs.

@vpelipenko
Copy link
Contributor

CR: passed
Builds: green
Resolution: OK to merge

vpelipenko added a commit that referenced this pull request Jan 8, 2015
Change interpret() return value to conform with Layout\ReaderInterface
@vpelipenko vpelipenko merged commit bc9d4ad into magento:develop Jan 8, 2015
@Vinai
Copy link
Contributor Author

Vinai commented Jan 8, 2015

Great! From other projects I have the experience they prefer small, atomic PRs.
But its easier for me to maintain a single one, too, so I'm happy to do combined ones in future.
Thanks for merging.

@vpelipenko
Copy link
Contributor

It depends on changes. In your case when you work on one issue (compatibility with Layout\ReaderInterface) and fix it in a few places it is preferable to have one PR. But if the changes are not related, and fix a few different issues, separate PR for each issue should be created.

@Vinai Vinai deleted the view-layout-reader-remove branch January 14, 2015 09:13
magento-team pushed a commit that referenced this pull request Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants