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

Bugfix/260 #261

Closed
wants to merge 5 commits into from
Closed

Bugfix/260 #261

wants to merge 5 commits into from

Conversation

evilnerd
Copy link
Contributor

@evilnerd evilnerd commented Mar 5, 2020

PR Description

This pull request fixes the issue added by me (#260) with 4 commits:

  • 2 commits to fix the issues mentioned (url pre-escaping and endless loop)
  • 2 commits to fix and add tests

Checklist

Dick Appel added 4 commits March 5, 2020 14:57
…ery. This is prevent it from going into an endless loop.
…re &maxResults=50 sent to a load balancer would result in no issues returned, whereas &maxResults=50 does return the correct results. This also works on non-loadbalanced environments.
…occurs, to show that now SearchPages doesn't go into an endless loop anymore.
@evilnerd
Copy link
Contributor Author

evilnerd commented Mar 6, 2020

Please hold, I'm testing this further and running into pagination issues.

@ghostsquad
Copy link
Contributor

@evilnerd when you are ready for review, please ensure you've rebased and your commits follow these rules:

https://conventionalcommits.org/en/v1.0.0-beta.4/#summary
https://chris.beams.io/posts/git-commit/#seven-rules

@evilnerd
Copy link
Contributor Author

@evilnerd when you are ready for review, please ensure you've rebased and your commits follow these rules:

https://conventionalcommits.org/en/v1.0.0-beta.4/#summary
https://chris.beams.io/posts/git-commit/#seven-rules

Hi, I've rebased (and it said I was up to date) but I'm not quite sure how to change the old commit messages. Should I?

@ghostsquad
Copy link
Contributor

@evilnerd sorry, I apologize for the lack of clarity, I mean perform a rebase and "fixup" your commits so that at the end of the rebase, you have 1 commit.

https://git-scm.com/docs/git-rebase

https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history

prior to rebasing, make sure you've pulled the latest changes in master locally.

@evilnerd evilnerd closed this Mar 13, 2020
@evilnerd evilnerd deleted the bugfix/260 branch March 13, 2020 12:01
@evilnerd
Copy link
Contributor Author

evilnerd commented Mar 13, 2020 via email

@ghostsquad
Copy link
Contributor

ghostsquad commented Mar 14, 2020

@evilnerd oh no! I'm so sorry you lost some of your work. Changing history can be destructive, so it's definitely not for the faint of heart. I didn't realize you were new to this operation, otherwise I might have suggested to backup the local repo (and thus your commits) prior to rebasing just in case.. ;)

I started using a tool to make conventional commits easier for me too:

https://github.com/vivaxy/gacp

I highly recommend it. There's still room for improvement of the tool, but it's a great starting point. Also Emoji's are awesome.

@ghostsquad
Copy link
Contributor

@evilnerd it looks like you closed this PR was that intentional? Were you going to submit a new PR?

I still see 5 commits in this PR.

If you would like, I can rebase for you, and squash them. I don't want to hang up this PR for too long, as I really appreciate the contributions. I also like linear and succinct Git history. :)

Let me know how I can help.

image

@evilnerd
Copy link
Contributor Author

evilnerd commented Mar 14, 2020 via email

@ghostsquad
Copy link
Contributor

@evilnerd oh I see. Ya, you have to git push --force in order to rewrite history of the branch that's in github.

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.

2 participants