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 GHRepository#listWorkflows() and add a test #1067

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

gsmet
Copy link
Contributor

@gsmet gsmet commented Mar 29, 2021

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 :).

.toIterable(GHWorkflow[].class, item -> item.wrapUp(root));
try {
GitHubRequest request = root.createRequest().withUrlPath(getApiTailUrl("actions/workflows")).build();
return new GHWorkflowsIterable(this, request);
Copy link
Member

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:

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().

Copy link
Contributor Author

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 :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

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.

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.

Minor change suggested.

@gsmet gsmet force-pushed the proper-workflow-list-test branch from 6caaf9e to c19cfd9 Compare March 30, 2021 17:57
@bitwiseman bitwiseman merged commit b069303 into hub4j:master Mar 30, 2021
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