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

fix language with spaces giving wrong results [#1607] #2038

Merged
merged 4 commits into from
Nov 13, 2019
Merged

fix language with spaces giving wrong results [#1607] #2038

merged 4 commits into from
Nov 13, 2019

Conversation

Dagizmo
Copy link
Contributor

@Dagizmo Dagizmo commented Oct 24, 2019

Fixes #1607

@Dagizmo Dagizmo changed the title fix language with spaces giving wrong results fix language with spaces giving wrong results [#1607] Oct 24, 2019
@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

Merging #2038 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #2038   +/-   ##
=======================================
  Coverage   70.73%   70.73%           
=======================================
  Files         537      537           
  Lines       14106    14106           
=======================================
  Hits         9978     9978           
  Misses       4128     4128
Impacted Files Coverage Δ
Octokit/Models/Response/PagesBuild.cs 0% <ø> (ø) ⬆️
Octokit/Models/Response/Installation.cs 0% <ø> (ø) ⬆️
Octokit/Clients/ProjectsClient.cs 95.91% <ø> (ø) ⬆️
Octokit.Reactive/Clients/ObservableSearchClient.cs 0% <ø> (ø) ⬆️
...tokit.Reactive/Clients/ObservableProjectsClient.cs 100% <ø> (ø) ⬆️
...ive/Clients/ObservableOrganizationMembersClient.cs 93.33% <ø> (ø) ⬆️
Octokit/Models/Request/OrganizationRequest.cs 83.33% <ø> (ø) ⬆️
Octokit/Clients/CheckSuitesClient.cs 94.73% <ø> (ø) ⬆️
Octokit/Helpers/ApiUrls.cs 98.62% <ø> (ø) ⬆️
Octokit/Models/Response/ApiError.cs 53.84% <ø> (ø) ⬆️
... and 16 more

@shiftkey
Copy link
Member

shiftkey commented Oct 31, 2019

@Dagizmo I think the only thing that's missing from this PR is a test that verifies we send through the space in the correct way (I don't think we need to encode it as %20 at this stage). Would you be able to add that in?

@Dagizmo
Copy link
Contributor Author

Dagizmo commented Oct 31, 2019

ok, will look into that.

@@ -117,7 +117,7 @@ public override IReadOnlyList<string> MergedQualifiers()

if (Language != null)
{
parameters.Add(string.Format(CultureInfo.InvariantCulture, "language:\"{0}\"", Language));
parameters.Add(string.Format(CultureInfo.InvariantCulture, "language:\"{0}\"", Language.ToParameter()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually found that this part didn't use to Parameter so named languages such as CPlusPlus was parsed as "CPlusPlus" instead of "cpp"

@Dagizmo
Copy link
Contributor Author

Dagizmo commented Nov 1, 2019

updated :D

Copy link
Member

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

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

@Dagizmo thanks for the contribution!

@shiftkey shiftkey merged commit fd6bca9 into octokit:master Nov 13, 2019
@shiftkey
Copy link
Member

shiftkey commented Feb 3, 2020

release_notes: address encoding of spaces when using Search API

@nickfloyd nickfloyd added Type: Bug Something isn't working as documented and removed category: bug labels Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search API: languages with space(s) fetch wrong results
3 participants