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

Moved some validations to the serializer #105

Merged
1 commit merged into from Jul 31, 2020
Merged

Moved some validations to the serializer #105

1 commit merged into from Jul 31, 2020

Conversation

marianoeramirez
Copy link
Contributor

@anx-cbenke
I created a new pull request with the same changes from the old pull request. because some changes were not intended to merge on this request.

These changes are only related to moving some of the validations to the serializer.

Copy link
Contributor

@anx-cbenke anx-cbenke left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you for your quick reaction! We have added some other PRs in the meanwhile, so there are a few conflicts now. Could you please resolve those, then this PR will be merged!

django_rest_passwordreset/views.py Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Jul 30, 2020

@marianoeramirez I would sugest to do an Interactive Rebase and squash the 7 commits into 1, this will make the history cleaner,

To fix the merge conflict you can rebase afterwards you change with the master branch. Make sure you do a force push on your branch afterwards or else git will reject the modification because of the change in the history.

@marianoeramirez
Copy link
Contributor Author

@anx-hnezbeda I am not sure if I did the squash right. I squash the commits into c7542f9.

Should I delete the old commits?

@ghost
Copy link

ghost commented Jul 30, 2020

@marianoeramirez you can try to follow this guide: https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

At the end you will have only 1 commit in your PR as all changes will be combined to one commit. Currently there are 11. Let me know if you need any more help with this 🙂

As mentioned before, you will need to to a force push to your branch as this is a change in the git history.

@marianoeramirez
Copy link
Contributor Author

@anx-hnezbeda I think now yes it's ok. let me know otherwise

@ghost
Copy link

ghost commented Jul 31, 2020

@marianoeramirez This looks much better 🙂, but there is one thing left. We merged some pull requests over the past days and this probably causes some conflicts on your side, which you have probably solved in the merge commit that was generated. To avoid this you can do a rebase with the master branch of the source repository. Here are the required steps:

  • Add the source remote: git remote add upstream https://github.com/anexia-it/django-rest-passwordreset.git
  • Fetch the recent changes merged into source: git fetch upstream
  • Remove the merge commit: git reset --hard HEAD~1
  • Rebase with the source master branch: git rebase upstream/master
  • Fix potential merge conflicts and continue rebase git rebase --continue (you will need this only if you have any conflicts during the rebase: https://docs.github.com/en/github/using-git/resolving-merge-conflicts-after-a-git-rebase)
  • Force push your changes: git push -f origin move_serializer

Here is a gist with some more details for such an approach: https://gist.github.com/ravibhure/a7e0918ff4937c9ea1c456698dcd58aa

@marianoeramirez
Copy link
Contributor Author

I think now yes is only one commit.

@ghost
Copy link

ghost commented Jul 31, 2020

@marianoeramirez looks great 👍

@ghost ghost merged commit 1116fb5 into anexia-it:master Jul 31, 2020
This pull request was closed.
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.

None yet

2 participants