Skip to content
This repository has been archived by the owner on Jul 28, 2022. It is now read-only.

Amqp remove deprecations #283

Merged

Conversation

makasim
Copy link
Contributor

@makasim makasim commented Nov 17, 2017

I am targeting this branch, because there is a BC-break.

Follow up #276

Changelog

### Removed
- The pull request removes previously deprecated stuff from the PR [#276](https://github.com/sonata-project/SonataNotificationBundle/pull/276). Now AMQP implementation relies on amqp interop and no longer coupled to php-amqplib. 

Subject

The pull request remove previously deprecated stuff from the PR: #276

@makasim makasim changed the base branch from 3.x to master November 17, 2017 07:04
@makasim makasim changed the title Amqp remove deprecations [WIP] Amqp remove deprecations Nov 17, 2017
@makasim
Copy link
Contributor Author

makasim commented Nov 17, 2017

@greg0ire Should I make classes final? properties private? while I am on it.

@greg0ire
Copy link
Contributor

Hey there!

It looks like you did not fill the pull request template properly. Maybe you
were confused, so let me walk you through this step by step.

Explaining why you target this branch

I am targeting this branch, because {reason}.

Here, you are supposed to replace {reason} with something like:

  • "there is a BC-break" if you picked the master branch
  • "there is no BC-break" if you picked the stable branch

Searching and finding related issues

Closes #{put_issue_number_here}

Please have a look at the list of issues and pull requests, and look for issues
that will be fixed, and pull requests that will become irrelevant should we
merge your PR. Each issue / pull request has a number shown right next to the
title. Kindly replace {put_issue_number_here} with whatever issue or PR is
relevant. Of course, you may specify several numbers.
On merge, these issues and PRs will automatically get
closed
.
If there is no issue or pull request that fits there, just remove the sentence
entirely.

Filling the changelog

That one is really important to get right, because it will be used to craft a
release changelog such as this
one
.
The first thing you need to know about this is that if your pull request is
pedantic, meaning it does not fix a bug for the user, or adds a new feature, it
is not needed. This is typically the case for PRs that fix the build or add
some documentation.

3 important things:

  1. Make sure to fill the changelog section with something directed at the end
    user, that answers the question "What benefit do I get from upgrading?".
  2. Make sure to remove empty sections.
  3. Make sure to keep the markdown markup.

Filling the TODO list

If you think your PR is ready, just remove it. If you have still some things
left to do, customize it.

Filling the Subject

Last but not least, describe your PR as well as you can. Bear in mind that some
reviewers do not even use the library they are maintaining, so try to be
crystal clear.

@greg0ire
Copy link
Contributor

@greg0ire Should I make classes final? properties private? while I am on it.

Sure! Just make sure to mention it the upgrade note, a bit like this: https://github.com/sonata-project/GoogleAuthenticator/blob/2.x/UPGRADE-2.0.md

@greg0ire greg0ire added the major label Nov 17, 2017
Copy link
Contributor

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Wow that's a lot less code, nice!

@makasim makasim changed the title [WIP] Amqp remove deprecations Amqp remove deprecations Nov 17, 2017
UPGRADE-4.0.md Outdated
### Closed API

* `AMQPBackendDispatcher` and `AMQPMessageIterator` classes are final. You cannot extend them.
* `AMQPBackend` properties are private now. You cannot overwrite them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Well technically, you can. It will just have no effect.

greg0ire
greg0ire previously approved these changes Nov 17, 2017
@makasim
Copy link
Contributor Author

makasim commented Nov 17, 2017

@greg0ire is there anything else that prevents us from merging it?

@greg0ire
Copy link
Contributor

Please address this: #283 (comment)

@greg0ire is there anything else that prevents us from merging it?

The lack of reviews. I try not to merge if I'm the only reviewer.
@sonata-project/contributors please review

@makasim
Copy link
Contributor Author

makasim commented Nov 17, 2017

@greg0ire is there anything else that prevents us from merging it?
done

@greg0ire
Copy link
Contributor

Please address that part: "Explaining why you target this branch"

@makasim
Copy link
Contributor Author

makasim commented Nov 17, 2017

Please address that part: "Explaining why you target this branch"

done

@greg0ire greg0ire merged commit c21ccec into sonata-project:master Nov 17, 2017
@greg0ire
Copy link
Contributor

Thanks @makasim !

@makasim makasim deleted the amqp-remove-deprecations branch November 17, 2017 13:05
@makasim makasim restored the amqp-remove-deprecations branch November 20, 2018 07:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants