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

[Notifier] Add LightSms notifier bridge #40607

Merged
merged 62 commits into from Apr 6, 2021
Merged

[Notifier] Add LightSms notifier bridge #40607

merged 62 commits into from Apr 6, 2021

Conversation

StaffNowa
Copy link
Contributor

@StaffNowa StaffNowa commented Mar 28, 2021

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR symfony/symfony-docs/pull/15178
Recipe PR symfony/recipes#921

LightSms notifier https://github.com/D4DLab/lightsms-notifier

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title * LightSms notifier [Notifier] * LightSms notifier Mar 28, 2021
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Some comments, but over all this looks great 👍🏻 thank you

EDIT:
It looks like your commiter email is not associated with your GitHub account

@StaffNowa
Copy link
Contributor Author

StaffNowa commented Mar 28, 2021

Do I need to make any action for this

Issues that cannot be fixed automatically
in Commits list, a git rebase is needed to remove any merges
Merge remote-tracking branch 'origin/5.x' into 5.x

Never used before

Possible I need to do this one step https://symfony.com/doc/current/contributing/code/pull_requests.html#step-4-submit-your-pull-request

Copy link
Contributor

@t-richard t-richard left a comment

Choose a reason for hiding this comment

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

Some rephrasing on the error messages

@OskarStark
Copy link
Contributor

It took time, but here we go, this is in now. Thank you very much Vasilij.

@OskarStark OskarStark merged commit 71a407d into symfony:5.x Apr 6, 2021
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Apr 6, 2021
This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[Notifier] [LightSMS] add docs

LightSMS symfony/symfony#40607

Commits
-------

3fe341f [Notifier] [LightSMS] add docs
@chalasr
Copy link
Member

chalasr commented Apr 6, 2021

Note: as a rule of thumb, commits should be squashed before merging (by the merger if not done by the contributor).

Screenshot 2021-04-06 at 15 34 42

@OskarStark
Copy link
Contributor

I know I already talked about this with with @jderusse and because the user changed its name in between it could not be squashed 😔

@StaffNowa
Copy link
Contributor Author

What should I read to reduce you work in the future? Maybe I can read something and make less work for you...

@azjezz
Copy link
Contributor

azjezz commented Apr 6, 2021

@StaffNowa instead of commiting additional changes in a separate commits, you can append your changes to the previous commit.

before:

$ git add src/Symfony/Notifier/Something.php
$ git commit -m"[Notifier] add something"
...
$ git add src/Symfony/Notifier/Tests/SomethingTest.php
$ git commit -m"[Notifier] add something tests"
$ git push

after:

$ git add src/Symfony/Notifier/Something.php
$ git commit -m"[Notifier] add something"
...
$ git add src/Symfony/Notifier/Tests/SomethingTest.php
$ git commit --amend --no-edit 
$ git push -f

now instead of having two commits, one with the message "[Notifier] add something" and the other with message "[Notifier] add something tests", you will end up with only 1 commit that has the message "[Notifier] add something"

@StaffNowa
Copy link
Contributor Author

Thanks. I will put for my remarks in the future ;)

@fabpot fabpot mentioned this pull request Apr 18, 2021
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.

None yet