Skip to content

Remove unused stream factory #9

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 3 commits into from
Jul 28, 2017
Merged

Remove unused stream factory #9

merged 3 commits into from
Jul 28, 2017

Conversation

kelunik
Copy link
Collaborator

@kelunik kelunik commented Jul 13, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? yes
License MIT

What's in this PR?

Remove the unused stream factory.

Why?

Because it's unused.

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.

Good. Just to make sure to add it in the Changelog.

kelunik added 2 commits July 13, 2017 16:14
This ensures we don't add another parameter which might be a BC break compared to v0.1.0 then.
@kelunik kelunik force-pushed the remove-stream-factory branch from 4bc010e to b860278 Compare July 13, 2017 14:16
@Nyholm
Copy link
Member

Nyholm commented Jul 13, 2017

There is no need to deprecate it. Just remove it and we release 0.2.0. But the change log should state that we removed it.

/**
* @param Artax\Client $client HTTP client implementation.
* @param ResponseFactory $responseFactory Response factory to use or `null` to attempt auto-discovery.
* @param StreamFactory $streamFactory This parameter will be ignored and removed in the next major version.
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove it directly withtout waiting for a next major and tag the next release a 0.2.0, it will be compliant with semver and as it's a fresh library it should be fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but that way we can tag v0.1.1 and nobody even has to update their composer.json.

@kelunik
Copy link
Collaborator Author

kelunik commented Jul 13, 2017

We can do that as well, don't really care.

@kelunik
Copy link
Collaborator Author

kelunik commented Jul 28, 2017

@Nyholm I'd just tag v0.1.1 for now, but if you really want to tag v0.2.0, I can change the PR, too.

@Nyholm
Copy link
Member

Nyholm commented Jul 28, 2017

Thank you for this PR.

@Nyholm Nyholm merged commit 78020ff into master Jul 28, 2017
@Nyholm
Copy link
Member

Nyholm commented Jul 28, 2017

Feel free to tag 0.1.1.

@Nyholm Nyholm deleted the remove-stream-factory branch July 28, 2017 16:35
@kelunik
Copy link
Collaborator Author

kelunik commented Jul 28, 2017

Tagged.

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