Skip to content

Conversation

@HazAT
Copy link
Member

@HazAT HazAT commented Oct 30, 2018

This is only a part of changes if I do everything in one go it's probably too much to review.

@HazAT HazAT self-assigned this Oct 30, 2018
@ste93cry ste93cry self-requested a review October 30, 2018 11:53
@HazAT HazAT changed the title [2.0] Public API changes [WIP] [2.0] Public API changes Oct 30, 2018
@HazAT HazAT changed the title [WIP] [2.0] Public API changes [2.0] Public API changes Nov 7, 2018
Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

The only blocking change that I see is the transport as a dep and not as a config.

Everything else is great! This seems a great simplification of a few things, and I'm happy to see that tests are largely untouched, PHPT too! 👍

Copy link
Contributor

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

Nice work, really! Just one suggestion for the future, not strictly related to this project: if you have such big changes to do try to address them as separate PRs that act on a small subset of the overall changes, otherwise the code review will be a mess and will be much more difficult for reviewers to understand. Also having smaller PRs means that the history of the project will be more explicative of what happened and why

@ste93cry ste93cry added this to the Release 2.0 milestone Nov 10, 2018
Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Overall it LGTM; I've found a few blocking issues, but all small.

I've opened two separate issues to track stuff afterwards and not here.

@ste93cry ste93cry merged commit fdb12a1 into 2.0 Nov 16, 2018
@ste93cry ste93cry deleted the feat/sdk-changes branch November 16, 2018 09:32
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.

4 participants