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: list value for GET search params #136

Merged
merged 3 commits into from Feb 14, 2023

Conversation

Kraigo
Copy link
Contributor

@Kraigo Kraigo commented Feb 9, 2023

1. Description

SearchParams for GET request should support list values.
Expected:
/request?id[]=123&id[]=456

Current implementation use these params into uri string as text. But then dart Uri.https() will add extra "?"(querstion mark sign) at end. That URL breaks API and server respond with 404 error.

Actual:

/request?id[]=123&id[]=456? 
__________________________^

Added special parsing for list query params.
Changed types to support list params.
Added extra tests to check response URL.

Example:
https://stackoverflow.com/questions/69633989/dart-how-to-send-query-parameter-array-with-brackets

1.1. Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples.

1.2. Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

1.3. Related Issues

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Merging #136 (d2c076d) into main (bc88dab) will not change coverage.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main     #136   +/-   ##
=======================================
  Coverage   73.83%   73.83%           
=======================================
  Files         105      105           
  Lines        1120     1120           
=======================================
  Hits          827      827           
  Misses        293      293           
Impacted Files Coverage Δ
lib/src/core/service_helper.dart 74.71% <100.00%> (ø)
...b/src/service/v1/accounts/accounts_v1_service.dart 97.20% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@myConsciousness
Copy link
Member

Hi @Kraigo , thanks for your contribution! :)

The implementation looks good, but there is one minor point that needs to be confirmed, please check :)

@Kraigo
Copy link
Contributor Author

Kraigo commented Feb 10, 2023

Hey @myConsciousness
Do you want me to check something, or you're going to do that?
Please give me more information :)

@myConsciousness
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 14, 2023

Build succeeded:

@bors bors bot merged commit f0894a6 into mastodon-dart:main Feb 14, 2023
myConsciousness added a commit that referenced this pull request Feb 15, 2023
bors bot added a commit that referenced this pull request Feb 15, 2023
138: docs: fixed for the pr (#136) r=myConsciousness a=myConsciousness

# 1. Description

<!-- Provide a description of what this PR is doing.
If you're modifying existing behavior, describe the existing behavior, how this PR is changing it,
and what motivated the change. If this is a breaking change, specify explicitly which APIs have been
changed. -->

## 1.1. Checklist

<!-- Before you create this PR confirm that it meets all requirements listed below by checking the
relevant checkboxes (`[x]`). This will ensure a smooth and quick review process. -->

- [x] The title of my PR starts with a [Conventional Commit] prefix (`fix:`, `feat:`, `docs:` etc).
- [x] I have read the [Contributor Guide] and followed the process outlined for submitting PRs.
- [x] I have updated/added tests for ALL new/updated/fixed functionality.
- [x] I have updated/added relevant documentation in `docs` and added dartdoc comments with `///`.
- [x] I have updated/added relevant examples in `examples`.

## 1.2. Breaking Change

<!-- Does your PR require users to manually update their apps to accommodate your change?

If the PR is a breaking change this should be indicated with suffix "!"  (for example, `feat!:`, `fix!:`). See [Conventional Commit] for details.
-->

- [ ] Yes, this is a breaking change.
- [x] No, this is _not_ a breaking change.

## 1.3. Related Issues

<!-- Provide a list of issues related to this PR from the [issue database].
Indicate which of these issues are resolved or fixed by this PR, i.e. Fixes #xxxx* !-->

<!-- Links -->

[issue database]: https://github.com/mastodon-dart/mastodon-api/issues
[contributor guide]: https://github.com/mastodon-dart/mastodon-api/blob/main/CONTRIBUTING.md
[style guide]: https://github.com/mastodon-dart/mastodon-api/blob/main/STYLEGUIDE.md
[conventional commit]: https://conventionalcommits.org


Co-authored-by: myConsciousness <contact@shinyakato.dev>
@myConsciousness
Copy link
Member

@all-contributors please add @Kraigo for doc and bug

@allcontributors
Copy link
Contributor

@myConsciousness

I've put up a pull request to add @Kraigo! 🎉

bors bot added a commit that referenced this pull request Feb 15, 2023
138: docs: fixed for the pr (#136) r=myConsciousness a=myConsciousness

# 1. Description

<!-- Provide a description of what this PR is doing.
If you're modifying existing behavior, describe the existing behavior, how this PR is changing it,
and what motivated the change. If this is a breaking change, specify explicitly which APIs have been
changed. -->

## 1.1. Checklist

<!-- Before you create this PR confirm that it meets all requirements listed below by checking the
relevant checkboxes (`[x]`). This will ensure a smooth and quick review process. -->

- [x] The title of my PR starts with a [Conventional Commit] prefix (`fix:`, `feat:`, `docs:` etc).
- [x] I have read the [Contributor Guide] and followed the process outlined for submitting PRs.
- [x] I have updated/added tests for ALL new/updated/fixed functionality.
- [x] I have updated/added relevant documentation in `docs` and added dartdoc comments with `///`.
- [x] I have updated/added relevant examples in `examples`.

## 1.2. Breaking Change

<!-- Does your PR require users to manually update their apps to accommodate your change?

If the PR is a breaking change this should be indicated with suffix "!"  (for example, `feat!:`, `fix!:`). See [Conventional Commit] for details.
-->

- [ ] Yes, this is a breaking change.
- [x] No, this is _not_ a breaking change.

## 1.3. Related Issues

<!-- Provide a list of issues related to this PR from the [issue database].
Indicate which of these issues are resolved or fixed by this PR, i.e. Fixes #xxxx* !-->

<!-- Links -->

[issue database]: https://github.com/mastodon-dart/mastodon-api/issues
[contributor guide]: https://github.com/mastodon-dart/mastodon-api/blob/main/CONTRIBUTING.md
[style guide]: https://github.com/mastodon-dart/mastodon-api/blob/main/STYLEGUIDE.md
[conventional commit]: https://conventionalcommits.org


Co-authored-by: myConsciousness <contact@shinyakato.dev>
Co-authored-by: Shinya Kato / 加藤 真也 <kato.shinya.dev@gmail.com>
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

3 participants