github-api icon indicating copy to clipboard operation
github-api copied to clipboard

Added listGists method

Open EasyG0ing1 opened this issue 3 years ago • 3 comments

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:

  • [X] 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.
  • [X] 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.
  • [X] Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
  • [X] Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

  • [X] Fill in the "Description" above with clear summary of the changes. This includes:
    • [X] 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."
  • [X] Enable "Allow edits from maintainers".

EasyG0ing1 avatar Jul 18 '22 16:07 EasyG0ing1

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%) :arrow_down:

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

codecov[bot] avatar Aug 13 '22 22:08 codecov[bot]

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));
            }
        }
    }

EasyG0ing1 avatar Aug 15 '22 13:08 EasyG0ing1

@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:

https://github.com/hub4j/github-api/blob/bcb71a3c318d243c92b757310cf153b8afc983d5/src/main/java/org/kohsuke/github/GHLicense.java#L196-L199 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 avatar Aug 18 '22 17:08 bitwiseman