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

Add site.github.public_repositories[].contributors #234

Merged

Conversation

9bow
Copy link
Contributor

@9bow 9bow commented Apr 29, 2022

Hello,

After I wrote #233, I saw a merged PR about site.github.public_repositories[].releases (#224 by @parkr )

I've never used ruby before, but with help of my friend @peniar , I made this PR. (thanks, @peniar)

Here're what I did:

First, I changed the code lib/jekyll-github-metadata/repository.rb in my installed gem, and check it worked.

And then, I wrote some spec code referring previous PR about release, assumed that return should be like spec/webmock/api_get_repo_contributors.json

Lastly, I tested with bundle exec rspec spec, and then 1 test I added failed as follows:

Failures:

  1) integration into a jekyll site calls all the stubs
     Failure/Error: expect(stub).to have_been_requested

       The request GET https://api.github.com/repos/jekyll/github-metadata/contributors?per_page=100 with headers {'Accept'=>/application\/vnd\.github\.(v3|drax-preview|mercy-preview)\+json/, 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token 1234abc', 'Content-Type'=>'application/json', 'User-Agent'=>'Octokit Ruby Gem 4.22.0'} was expected to execute 1 time but it executed 2 times

I think the error expected to execute 1 time but it executed 2 times is because the line 16 in spec/spec_helpers/stub_helper.rb conflict with line 26 which I added. However, line 15 for releases code does not conflict with line 25. :(

I'm stuck here and don't know what to try or look for anymore.

If anyone could point out this PR, I would be very grateful.

Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Definite 👍 to the idea... wondering if we can use the Value class to improve the performance / reduce API calls if we aren't going to use these fields.

lib/jekyll-github-metadata/repository.rb Outdated Show resolved Hide resolved
Co-authored-by: Parker Moore <237985+parkr@users.noreply.github.com>
@9bow
Copy link
Contributor Author

9bow commented Apr 29, 2022

Thanks for your review, @parkr

How about the following method? I made it work only when JEKYLL_GITHUB_METADATA_DETAILS is set to true by using an environment variable.
If you have any ideas on how to pass this flag or name convention, it would be nice if you could let me know. :)

      def owner_public_repositories
        options = {
          :type   => "public",
          :accept => "application/vnd.github.mercy-preview+json",
        }
        if ENV["JEKYLL_GITHUB_METADATA_DETAILS"] == true
          memoize_value :@owner_public_repositories, Value.new("owner_public_repositories", proc do |c|
            c.list_repos(owner, options).each do |r|
              r[:releases] = c.releases(r[:full_name])
              r[:contributors] = c.contributors(r[:full_name], options)
            end
          end)
        else
          memoize_value :@owner_public_repositories, Value.new("owner_public_repositories", proc { |c| c.list_repos(owner, options) })
        end
      end

@parkr
Copy link
Member

parkr commented May 2, 2022

I was able to get this to pass on my machine with the following change:

diff --git lib/jekyll-github-metadata/repository.rb lib/jekyll-github-metadata/repository.rb
index 1e4864c..9f75d2f 100644
--- lib/jekyll-github-metadata/repository.rb
+++ lib/jekyll-github-metadata/repository.rb
@@ -118,9 +118,10 @@ module Jekyll
           :accept => "application/vnd.github.mercy-preview+json",
         }
         memoize_value :@owner_public_repositories, Value.new("owner_public_repositories", proc do |c|
-          c.list_repos(owner, options).each do |r| 
-            r[:releases] = c.releases(r[:full_name])
-            r[:contributors] = c.contributors(r[:full_name], options)
+          c.list_repos(owner, options).map do |r|
+            r[:releases] = Value.new("owner_public_repositories_releases", proc { c.releases(r[:full_name]) })
+            r[:contributors] = Value.new("owner_public_repositories_contributors", proc { c.contributors(r[:full_name]) })
+            r
           end
         end)
       end
diff --git lib/jekyll-github-metadata/sanitizer.rb lib/jekyll-github-metadata/sanitizer.rb
index edaf554..0a993d3 100644
--- lib/jekyll-github-metadata/sanitizer.rb
+++ lib/jekyll-github-metadata/sanitizer.rb
@@ -16,7 +16,7 @@ module Jekyll
       # resource - an Object
       #
       # Returns the sanitized resource.
-      # rubocop:disable Metrics/CyclomaticComplexity
+      # rubocop:disable Metrics/CyclomaticComplexity, Metrics/MethodLength
       def sanitize(resource)
         case resource
         when Array
@@ -31,6 +31,8 @@ module Jekyll
           nil
         when String
           resource
+        when Value
+          resource.render
         else
           if resource.respond_to?(:to_hash)
             sanitize_resource(resource)
@@ -39,7 +41,7 @@ module Jekyll
           end
         end
       end
-      # rubocop:enable Metrics/CyclomaticComplexity
+      # rubocop:enable Metrics/CyclomaticComplexity, Metrics/MethodLength
 
       # Sanitize the Sawyer Resource or Hash
       # Note: the object must respond to :to_hash for this to work.

I don't think we want to make this an optional change – we can just load only when requested using Value.

@9bow
Copy link
Contributor Author

9bow commented May 3, 2022

Thanks for the detailed guide, @parkr!

As per your guide, I wrapped the existing codes to use Value.new() and changed the Sanitizer module as well. I definetly agree that we do not want to make this an optional change, but honestly I'm still not sure what Value does. 😅 I think I need to study the whole code as well as the syntax of ruby more.

Anyway, please let me know if there is anything else I need to change or do.

@9bow
Copy link
Contributor Author

9bow commented May 3, 2022

(TMI: My friend @peniar received e-mail when I commented, and he also taught me how Value works and how/why to use it. Thanks, @peniar!)

Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Two quick rubocop failures. Thank you!

@@ -23,6 +23,7 @@ def stub_all_api_requests

owner_repos = JSON.parse(webmock_data("owner_repos"))
owner_repos.each { |r| stubs << stub_api("/repos/#{r["full_name"]}/releases?per_page=100", "repo_releases") }
owner_repos.each { |r| stubs << stub_api("/repos/#{r["full_name"]}/contributors?per_page=100", "repo_contributors") }
Copy link
Member

Choose a reason for hiding this comment

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

From Rubocop:

Style/CombinableLoops: Combine this loop with the previous loop.

expect(subject["public_repositories"].first).to have_key("contributors")
expect(subject["public_repositories"].first["contributors"].size).to eql(1)
expect(subject["public_repositories"].first["contributors"].first["login"]).to eql("parkr")
expect(subject["public_repositories"].first["contributors"].first["id"]).to eql(237985)
Copy link
Member

Choose a reason for hiding this comment

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

Rubocop is failing on this line. I think it's unnecessary so I'd recommend that you remove this line.

@9bow
Copy link
Contributor Author

9bow commented May 4, 2022

Thanks for reviewing this. I just removed spec related codes, and am waiting for CI results!

@parkr
Copy link
Member

parkr commented May 4, 2022

@9bow Sorry, I just meant the one line, not all your specs! I like your tests. Just remove the one line I pointed out, and combine the loop you added in spec_helper with the one right above it.

@9bow
Copy link
Contributor Author

9bow commented May 4, 2022

Oh, I'm sorry @parkr. I misunderstood.
I'll revive those Spec codes and re-apply your review.

@9bow
Copy link
Contributor Author

9bow commented May 4, 2022

I've been re-applied your review. Also verified that it passed the test locally as follows:
image

Sorry for the hassle and thank you for your kind and patient review!

Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

Thank you!

@parkr
Copy link
Member

parkr commented May 5, 2022

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 421dd6e into jekyll:main May 5, 2022
@9bow
Copy link
Contributor Author

9bow commented May 5, 2022

Thank you very much, @parkr!
With your review, this PR was a great experience for me. :)

@9bow
Copy link
Contributor Author

9bow commented May 5, 2022

Could you please let me know when the next release schedule is?
I'd love to try out this new feature. 😄

@parkr
Copy link
Member

parkr commented May 5, 2022

v2.14.0 has been released!

@9bow
Copy link
Contributor Author

9bow commented May 5, 2022

Awesome! Thanks a lot @parkr!
However, when I try to apply it, a conflict with the dependency of the github-pages gem.
Could you please help on this part?

@9bow
Copy link
Contributor Author

9bow commented May 7, 2022

I've finally rolled out this cool new feature on our site! 🎉
I made it automatically show repositories from @PyTorchKorea, which has 2 or more contributors
Unfortunately, github/pages-gem dependency is not updated yet, so I'm referring to my personal repository.

Again, thank you very much, @parkr!

@9bow 9bow deleted the impl/add-each-contributors-for-public_repos branch May 7, 2022 09:39
@jekyll jekyll locked and limited conversation to collaborators May 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants