Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Escape "query" parameter #121

Merged
merged 1 commit into from
Jan 10, 2022
Merged

Escape "query" parameter #121

merged 1 commit into from
Jan 10, 2022

Conversation

thinca
Copy link
Contributor

@thinca thinca commented Jan 6, 2022

"query" parameter may contains any characters such as "&".

Sorry for no tests.
I'm newbie for Go. Adding tests about them was difficult for me. 🙇

@sivchari
Copy link
Owner

sivchari commented Jan 8, 2022

Thank you for PR 😄, good catch !
Please update target branch dev-1.1 to main. I wanna apply these changes for gotwtr immediately.

"query" parameter may contains any characters such as "&".
@thinca thinca changed the base branch from dev-1.1 to main January 8, 2022 17:42
@thinca
Copy link
Contributor Author

thinca commented Jan 8, 2022

OK. I rebased.
Note that searchAllTweets API does not not exist in main. Therefore, you need to fix it in the dev-1.1 branch.

@sivchari
Copy link
Owner

sivchari commented Jan 8, 2022

@thinca

Note that searchAllTweets API does not not exist in main. Therefore, you need to fix it in the dev-1.1 branch.

Yes, I know. I'll create PR for apply for your comment 👍

@thinca
Copy link
Contributor Author

thinca commented Jan 9, 2022

CI failed, but the test succeeds in my local environment.
It appears to be a cache issue in the CI environment.

@sivchari
Copy link
Owner

sivchari commented Jan 9, 2022

I used act (demonstrate github actions in local), and this CI is passed. So, I don't no idea that why is this checks is failed. You said cache issue in the CI environment, but we cached only go.sum. As these reason, the checks is failed, but I'd like to merge this PR. What do you think ?

@thinca
Copy link
Contributor Author

thinca commented Jan 10, 2022

Fmm... Sorry, this is not the cache problem.

https://github.com/sivchari/gotwtr/pull/121/checks#step:2:205

HEAD is now at 937d90b Merge b9c61bf into 749a04a

749a04a is a commit at dev-1.1. The CI is not updating the merge point.

I'll try update it by close/reopen this PR.

@thinca thinca closed this Jan 10, 2022
@thinca thinca reopened this Jan 10, 2022
@thinca
Copy link
Contributor Author

thinca commented Jan 10, 2022

Oh... re-approval is needed.

@sivchari
Copy link
Owner

Hi, I comprehended, so I approve now.

@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #121 (b9c61bf) into main (0743c3c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #121   +/-   ##
=======================================
  Coverage   56.99%   56.99%           
=======================================
  Files          30       30           
  Lines        1493     1493           
=======================================
  Hits          851      851           
  Misses        406      406           
  Partials      236      236           
Impacted Files Coverage Δ
search_spaces.go 44.44% <100.00%> (-1.99%) ⬇️
search_tweet.go 46.15% <100.00%> (-1.35%) ⬇️
space_option.go 38.88% <100.00%> (+1.15%) ⬆️
tweet_counts.go 44.44% <100.00%> (-1.99%) ⬇️
tweet_option.go 41.46% <100.00%> (+0.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0743c3c...b9c61bf. Read the comment docs.

@sivchari sivchari merged commit f133f86 into sivchari:main Jan 10, 2022
@thinca thinca deleted the escape-query-parameter branch January 10, 2022 11:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants