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

Added listGists method #1490

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Added listGists method #1490

wants to merge 4 commits into from

Conversation

EasyG0ing1
Copy link
Contributor

@EasyG0ing1 EasyG0ing1 commented Jul 18, 2022

Description

Currently, when pulling down a list of GHGist objects, using

gitHub.getMyself().listGists().toList()

The GHGist objects returned in the list do not contain the contents of the files within each Gist, and there is no other method from which to pull down all gists with complete files in a single list. The reason for this seems to be that the GitHub API URL path that is used to pull the list from, intentionally does not include the contents of the file. The API docs seem to indicate a conservative data yield for that particular path, requiring a developer to then use another URL path from which to pull the individual Gists, which would then include the contents of the file.

I added this method to the GitHub class, which I have tested and verified that the list it returns is a complete list of the user's GHGist objects, where each file within the Gists has all of the data associated with the file, including content.

Fixes #1325

The only issue that came from mvn -D enable-ci clean install site was a warning about inconsistent synchronization, but it didn't just flag my code, it flagged other code in the class as well. It also indicates that the code might be just fine and that it could be a false positive due to the nature of the test.

The method is relatively simple, I cannot see where it would cause any issues. However, I'm willing to make whatever changes are necessary.

Before submitting a PR:

  • Changes must not break binary backwards compatibility. If you are unclear on how to make the change you think is needed while maintaining backward compatibility, CONTRIBUTING.md for details.
  • Add JavaDocs and other comments as appropriate. Consider including links in comments to relevant documentation on https://docs.github.com/en/rest .
  • Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
  • Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

  • Fill in the "Description" above with clear summary of the changes. This includes:
    • If this PR fixes one or more issues, include "Fixes #" lines for each issue.
    • Provide links to relevant documentation on https://docs.github.com/en/rest where possible.
  • All lines of new code should be covered by tests as reported by code coverage. Any lines that are not covered must have PR comments explaining why they cannot be covered. For example, "Reaching this particular exception is hard and is not a particular common scenario."
  • Enable "Allow edits from maintainers".

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.

Please add at least one test for your new method.

@codecov
Copy link

codecov bot commented Aug 13, 2022

Codecov Report

Merging #1490 (aad672c) into main (262cf84) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #1490      +/-   ##
============================================
- Coverage     78.81%   78.75%   -0.07%     
  Complexity     2114     2114              
============================================
  Files           202      202              
  Lines          6429     6434       +5     
  Branches        361      362       +1     
============================================
  Hits           5067     5067              
- Misses         1152     1157       +5     
  Partials        210      210              
Impacted Files Coverage Δ
src/main/java/org/kohsuke/github/GitHub.java 80.35% <0.00%> (-1.84%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bitwiseman bitwiseman self-requested a review August 13, 2022 22:53
@EasyG0ing1
Copy link
Contributor Author

EasyG0ing1 commented Aug 15, 2022

Please add at least one test for your new method.

The test will need to use an account that has Gists and files in the Gists in order to work. I wrote a test that authenticates with my own token and it works, but I don't know how to write the test so that it will always work regardless of who is running it.

This test works, but it will only work when a valid token has been given to it.

    @Test
    public void gistListWithFilesTest() throws Exception {
        GitHub gh = new GitHubBuilder().withOAuthToken("<valid token>").build();
        List<GHGist>  ghGistList = gh.listGists();
        assertThat(ghGistList, notNullValue());
        for(GHGist ghGist : ghGistList) {
            assertThat(ghGist.getFiles(), notNullValue());
            for(GHGistFile gistFileContent : ghGist.getFiles().values()) {
                boolean hasContent = gistFileContent.getContent().length() > 0;
                assertThat(hasContent, is(true));
            }
        }
    }

@bitwiseman
Copy link
Member

bitwiseman commented Aug 18, 2022

@EasyG0ing1

You can record a snapshot of the tests and then add the snapshotNotAllowed(); method to prevent it from being overwritten later.

However, I'm going o make another suggestion. Instead of adding a new method to get the list Gists with contents, I suggest you add a populate() method to GHGist that calls the URL that returns the full contents similar to other populate() methods:

URL url = getUrl();
if (url != null) {
root().createRequest().setRawUrlPath(url.toString()).fetchInto(this);
}

That will allow people to get the file contents if needed while preserving the incremental loading. The test will also be simpler. You can add a step at the end of the existing test for listing Gists.

@bitwiseman bitwiseman marked this pull request as draft September 2, 2022 06:09
@bitwiseman bitwiseman added the work-abandoned There hasn't been any activity on the PR in a while. Another contributor might want to pick it up. label Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-abandoned There hasn't been any activity on the PR in a while. Another contributor might want to pick it up.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessing Gists via getMyself().listGists() returns null data for Gist files
2 participants