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

Split pull request search from issue; extend with filters #1693

Merged

Conversation

kgromov
Copy link
Contributor

@kgromov kgromov commented Aug 11, 2023

Enhancement

  • Add pull request search implementation to split from issues - GHPullRequestSearchBuilder
  • Extend with most of search parameters according to documentation
  • Added to both GHRepository and GitHub client itself - so it's possible to search for pull requests from certain repository or in general for organization: GHRepository#searchPullRequests(GHPullRequestSearchBuilder search) and GitHub#searchPullRequests accordingly.

Inspiration for this extension was own project with custom filters grouped differently to what github UI prvides (and a bit more user intuitive). And as it turned out library did not have a lot of search parameters in order to shorten search and response time (like created/merged dates and ranges, labels etc).

@kgromov
Copy link
Contributor Author

kgromov commented Aug 16, 2023

What I'm doing wrong? Since on my local env mvn spotless:apply build is successful for both java11 & 17:
Screenshot 2023-08-17 002747

@bitwiseman
Copy link
Member

Did you commit and push any updates? I just did so.

@kgromov
Copy link
Contributor Author

kgromov commented Aug 16, 2023

Did you commit and push any updates? I just did so.

Sorry my bad - didn't notice those changes in javadoc

@bitwiseman
Copy link
Member

@kgromov
Looks like there's some test failures now.

@kgromov
Copy link
Contributor Author

kgromov commented Aug 18, 2023

@kgromov Looks like there's some test failures now.

I'll take a look.
New tests were passed in -Dtest.github.useProxy and -Dtest.github.org=false -Dtest.github.takeSnapshot modes respectively.
Should I request access to hub4j-test-org or it's not necessary and snapshots can be taken from own repository?

@bitwiseman
Copy link
Member

Snapshots can be taken from anywhere, but we prefer the test org for later rerecording if needed.

It shouldn't cause failures though.

@bitwiseman
Copy link
Member

bitwiseman commented Aug 19, 2023

Again, you need to check in files created during takeSnapshot.

@kgromov
Copy link
Contributor Author

kgromov commented Aug 24, 2023

Again, you need to check in files created during takeSnapshot.

@bitwiseman can you please trigger build - I've updated snapshots few days ago

@kgromov
Copy link
Contributor Author

kgromov commented Aug 25, 2023

Again, you need to check in files created during takeSnapshot.

@bitwiseman can you please trigger build - I've updated snapshots few days ago

And again please (hopefully last time) - I missed one obvious part that wasn't specified in the CONTRIBUTING.md 😊

@bitwiseman
Copy link
Member

Please update the contributing doc with the instructions that were missing. Thanks!

@kgromov
Copy link
Contributor Author

kgromov commented Aug 25, 2023

Please update the contributing doc with the instructions that were missing. Thanks!

Done (with test data fix)

@kgromov
Copy link
Contributor Author

kgromov commented Aug 31, 2023

Hi, @bitwiseman.
Following existing pattern I've added org.kohsuke.github.GHPullRequestSearchBuilder to exclusion for code coverage (since it's also has setters for complex GH* objects) and implies interaction with Requester:

<exclude>org.kohsuke.github.GHPullRequestReviewBuilder.DraftReviewComment</exclude>
                      <exclude>org.kohsuke.github.GHIssue.PullRequest</exclude>
                      <exclude>org.kohsuke.github.GHCommitSearchBuilder</exclude>
                      <exclude>org.kohsuke.github.GHRepositorySearchBuilder</exclude>
                      <exclude>org.kohsuke.github.GHUserSearchBuilder</exclude>

Is it fine or should I cover all methods for new class?

@bitwiseman
Copy link
Member

I'll take a look shortly.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Files Coverage Δ
src/main/java/org/kohsuke/github/GHRepository.java 72.82% <100.00%> (+1.07%) ⬆️
src/main/java/org/kohsuke/github/GitHub.java 82.89% <100.00%> (+0.07%) ⬆️
...org/kohsuke/github/GHPullRequestSearchBuilder.java 91.11% <91.11%> (ø)

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!

@kgromov
Copy link
Contributor Author

kgromov commented Sep 22, 2023

@bitwiseman, can you trigger builds? I guess last commit was fine (I extended code coverage to 91%) - I just forgot, that java.version is 8

@kgromov
Copy link
Contributor Author

kgromov commented Nov 5, 2023

Hi, @bitwiseman. Any news when PR can be merged, thanks?

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

Great job on the test coverage!
This is very close. Last couple questions.

@kgromov
Copy link
Contributor Author

kgromov commented Nov 11, 2023

Great job on the test coverage! This is very close. Last couple questions.

Updated - please take a look last commit

@kgromov
Copy link
Contributor Author

kgromov commented Nov 12, 2023

Great job on the test coverage! This is very close. Last couple questions.

Updated - please take a look last commit

UPD: rolled back changed type for terms. I would personally make it LinkedHashSe to guarantee terms uniqueness (hopefully it's not affected github API but can add some ambiguity for wirewock mappings):

public abstract class GHSearchBuilder<T> extends GHQueryBuilder<T> {

    /** The terms. */
    protected final List<String> terms = new ArrayList<String>();

@bitwiseman
Copy link
Member

Great job on the test coverage! This is very close. Last couple questions.

Updated - please take a look last commit

UPD: rolled back changed type for terms. I would personally make it LinkedHashSe to guarantee terms uniqueness (hopefully it's not affected github API but can add some ambiguity for wirewock mappings):

public abstract class GHSearchBuilder<T> extends GHQueryBuilder<T> {

    /** The terms. */
    protected final List<String> terms = new ArrayList<String>();

That seems like a reasonable change, but maybe make that in a separate PR after we land this one?

@kgromov
Copy link
Contributor Author

kgromov commented Nov 14, 2023

Great job on the test coverage! This is very close. Last couple questions.

Updated - please take a look last commit

UPD: rolled back changed type for terms. I would personally make it LinkedHashSe to guarantee terms uniqueness (hopefully it's not affected github API but can add some ambiguity for wirewock mappings):

public abstract class GHSearchBuilder<T> extends GHQueryBuilder<T> {

    /** The terms. */
    protected final List<String> terms = new ArrayList<String>();

That seems like a reasonable change, but maybe make that in a separate PR after we land this one?

yes, since it's related to all search builders

@bitwiseman bitwiseman merged commit 9ff3264 into hub4j:main Nov 15, 2023
9 checks passed
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