-
Notifications
You must be signed in to change notification settings - Fork 724
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 GHRepository#listWorkflows() and add a test #1067
Conversation
.toIterable(GHWorkflow[].class, item -> item.wrapUp(root)); | ||
try { | ||
GitHubRequest request = root.createRequest().withUrlPath(getApiTailUrl("actions/workflows")).build(); | ||
return new GHWorkflowsIterable(this, request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this iterable only points to "actions/workflows" endpoint, you can late resolve the URL like we do here:
github-api/src/main/java/org/kohsuke/github/GHSearchBuilder.java
Lines 47 to 54 in 6caaf9e
public PagedSearchIterable<T> list() { | |
req.set("q", StringUtils.join(terms, " ")); | |
try { | |
return new PagedSearchIterable<>(root, req.build(), receiverType); | |
} catch (MalformedURLException e) { | |
throw new GHException("", e); | |
} |
You can even pass only owner
and create the requester in _iterator()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely convinced given I find the current coding more consistent with the rest of the codebase but as you wish :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yeah. Most other list*
methods throw IOException
. Since you made this method not throw IOException
(which I like) I'd rather have that method really not throw.
It's probably silly, but thanks for being accommodating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor change suggested.
6caaf9e
to
c19cfd9
Compare
I don't know how I missed it but I forgot to add a test for
listWorkflows()
and... it didn't work properly.I think we probably need a better infrastructure to deal with all these iterables to avoid having to add so many classes but that's something we can discuss another day :).