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

Don't add state query param twice #264

Merged
merged 2 commits into from Apr 11, 2022

Conversation

passsy
Copy link
Contributor

@passsy passsy commented Sep 14, 2021

I was wondering why the direction argument wasn't working for github.pullRequests.list(). Turns out a double ? was added in the URI.

Screen-Shot-2021-09-14-17-52-59 39

@timmaffett
Copy link

I am new to this library, but in evaluating the code and the repository activity I found it interesting that this PR has not been merged into the code yet.. This is clearly a bug, and this is clearly a correct fix:

Stream<PullRequest> list(
    RepositorySlug slug, {
    int? pages,
    String? base,
    String direction = 'desc',
    String? head,
    String sort = 'created',
    String state = 'open',
  }) {
    final params = <String, dynamic>{};
    putValue('base', base, params);
    putValue('direction', direction, params);
    putValue('head', head, params);
    putValue('sort', sort, params);
    putValue('state', state, params);   ///<<ALWAYS added to params here, so it will get included

    return PaginationHelper(github).objects(
        'GET',
        '/repos/${slug.fullName}/pulls?state=$state',  //<< and this '?state=$state' at end will always result in bad url 
                                                                                 //<<when query params are subsequently added in GitHub.request()
        (dynamic i) => PullRequest.fromJson(i),
        pages: pages,
        params: params);
  }
  • The state is always added to the query params (putValue('state', state, params);)
  • The params object is always passed to the PaginationHelper()
  • and that URL of '/repos/${slug.fullName}/pulls?state=$state' , with the '?state=$state' appended onto the end, will always result in an incorrect URL when the param object eventually passed to GitHub.request() and the params is used to build a query string and it is appended to the path...

@passsy
Copy link
Contributor Author

passsy commented Apr 11, 2022

@robbecker-wf, @CaseyHillers?

@CaseyHillers CaseyHillers merged commit e8b0e14 into SpinlockLabs:master Apr 11, 2022
@robbecker-wf robbecker-wf added the semver:patch Bug fixes, no public API changes label Apr 11, 2022
@robbecker-wf
Copy link
Contributor

Released in https://github.com/SpinlockLabs/github.dart/releases/tag/9.1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch Bug fixes, no public API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants