Some Improvements #32

ileodo · 2021-01-31T03:59:49Z

Fixed the gh search to support Github Search syntax.
Support different TTL for the cache of repo/org/pull
Add an option that search all the PR you are involved (regardless the organisation)
Parameterise the actions in the workflow.
Support fetching multiple pages when caching user_repo, org, pull_request

ileodo · 2021-01-31T04:03:36Z

lib/data_source/client/github.rb

+        return cache if cache
+        ret = block.call
+        write_cache(filename, JSON.dump(ret[:cache_content])) if ret[:write]
+        ret[:cache_content]


purely deal with cache logic, move out the handle_response logic.

ileodo · 2021-01-31T04:05:35Z

app.rb

-        access_token: ENV['GITHUB_ACCESS_TOKEN'],
-        cache_dir: ENV['alfred_workflow_cache']
-      )
+          host: ENV['GITHUB_API_HOST'] || 'api.github.com',


change to GITHUB_API_HOST , to differentiate it from the web UI host.

ileodo · 2021-01-31T04:05:54Z

assets/info.plist

@@ -232,7 +232,7 @@
 				<key>spaces</key>
 				<string></string>
 				<key>url</key>
-				<string>https://github.com/notifications</string>
+				<string>https://{var:GITHUB_HOST}/notifications</string>


parameterise the url in the action

ileodo · 2021-01-31T04:05:59Z

assets/info.plist

@@ -395,7 +355,100 @@ fi</string>
 				<key>spaces</key>
 				<string></string>
 				<key>url</key>
-				<string>https://github.com/settings/tokens/new?description=Github%20Repos%20Alfred%20workflow&amp;scopes=repo</string>
+				<string>https://{var:GITHUB_HOST}/settings/tokens/new?description=Github%20Repos%20Alfred%20workflow&amp;scopes=repo</string>


parameterise the url in the action

ileodo · 2021-01-31T04:06:18Z

assets/info.plist

 		<string>api.github.com</string>
+		<key>GITHUB_HOST</key>
+		<string>www.github.com</string>


web ui host.

ileodo · 2021-01-31T04:08:27Z

lib/commands/user_repos.rb

@@ -13,7 +13,7 @@ def initialize(repositories:)
    end

    def call(args)
-      results = filter_repos(args[0])
+      results = filter_repos(args.join(" "))


ileodo · 2021-01-31T04:08:59Z

lib/data_source/client/github.rb

-          request('/user/repos', params)
+        with_cache(:user_repos) do
+
+          page_count = get_total_page_for_request('/user/repos', params)


get page count first.

ileodo · 2021-01-31T04:09:23Z

lib/data_source/client/github.rb

+            end
+            {write: write, cache_content: all_user_repos}
+          else
+            res = request('/user/repos', params)


fallback, just incase page count fetching failed.

ileodo · 2021-01-31T04:10:12Z

lib/data_source/client/github.rb

+                all_user_repos = all_user_repos+ deserialize_body(part_res.body)
+              end
+            end
+            {write: write, cache_content: all_user_repos}


now the cache component interface is taking object as argument instead of string.

ileodo · 2021-01-31T04:11:00Z

lib/data_source/client/github.rb

+        raise res[:message] unless res.is_a?(Net::HTTPSuccess)
+
+        begin
+          res.header["Link"].split(",").map do |result|


fetch the total page number from the header "Link"

edgarjs

Thanks for doing these changes @ileodo! I left a few comments, and also please add the corresponding unit tests.

edgarjs · 2021-02-01T16:30:13Z

lib/data_source/client/github.rb

+            sort: 'pushed',
+            direction: 'desc',
+            per_page: 100


I think this indentation got hit twice.

edgarjs · 2021-02-01T16:33:17Z

lib/data_source/client/github.rb

        params = search_params('', modifiers).merge(
-          per_page: 100
+            per_page: 100


Also indentation has 4 spaces here

edgarjs · 2021-02-01T19:01:50Z

lib/data_source/client/github.rb

+          if page_count != nil
+            all_user_pulls = Array.new
+            write = true
+            (1..page_count).step(1) do |n|
+              params[:page] = n
+              part_res = request('/search/issues', params)
+              if not part_res.is_a?(Net::HTTPSuccess)
+                write = false
+                break
+              else
+                all_user_pulls = all_user_pulls+ deserialize_body(part_res.body)
+              end
+            end
+            {write: write, cache_content: all_user_pulls}
+          else
+            res = request('/search/issues', params)
+            if not res.is_a?(Net::HTTPSuccess)
+              { write: false, cache_content: nil }
+            else
+              { write: true, cache_content: deserialize_body(res.body) }
+            end


This block seems identical to the one in the other method (user_repos). Do you think we can refactor it? I would suggest avoid using too many if branches as well. You can use early returns to achieve that ;)

Thanks for reviewing @edgarjs , I updated the PR with some refactoring and unit tests.

I'm not super familiar with the ruby language feature, open to any suggestion change. Thanks again.

edgarjs

@ileodo I reviewed the changes and left some comments. I'm still have pending to review the rest of the files. Thank you for all your help!

edgarjs · 2021-02-03T16:34:18Z

lib/data_source/client/github.rb

+        if @pr_all_involve_me
+          modifiers = ['is:pr', 'state:open', "involves:#@me_account"]
+        else
+          modifiers = org_modifiers('is:pr', "user:#@me_account", 'state:open', "involves:#@me_account")
+        end
        params = search_params('', modifiers).merge(
          per_page: 100
        )


I would move the if statement to another method called pulls_modifiers and just call it directly in search_params:

Suggested change

if @pr_all_involve_me

modifiers = ['is:pr', 'state:open', "involves:#@me_account"]

else

modifiers = org_modifiers('is:pr', "user:#@me_account", 'state:open', "involves:#@me_account")

end

params = search_params('', modifiers).merge(

per_page: 100

)

params = search_params('', pulls_modifiers).merge(

per_page: 100

)

lib/data_source/client/github.rb

edgarjs · 2021-02-03T17:11:54Z

lib/data_source/client/github.rb

+        if res.key?("Link")
+          res["Link"].split(",").map do |result|
+            page_num, rel = result.match(/&page=(\d+)>; .*?"(\w+)"/i).captures
+            page_count = page_num.to_i if rel == "last"
+          end
+        end


This is a clever solution! Thank you. I'll just add my two cents: Instead of doing a map, I would use find, so that when the item you're looking is found, it stops the iteration. Although is possible that the rel="last" item is the last one, in which case maybe it's worth doing a reverse: res['Link'].split(',').reverse.find do |result|.

And also I would move this logic to another method called results_last_page(res)

edgarjs · 2021-02-03T17:15:17Z

lib/data_source/client/github.rb

+            result = nil
+            break
+          else
+            result.append deserialize_body(part_res.body)


This is good, but since you mentioned you're new to the Ruby language, another way to do it in Ruby is result << deserialize_body(part_res.body).

But you don't need to change it, just FYI.

edgarjs · 2021-02-03T17:19:42Z

lib/data_source/client/github.rb

+          if not res.is_a?(Net::HTTPSuccess)
+            nil
+          else
+            deserialize_body(res.body)
+          end


We should return an empty array if response is not successful.

Suggested change

if not res.is_a?(Net::HTTPSuccess)

nil

else

deserialize_body(res.body)

end

res.is_a?(Net::HTTPSuccess) ? deserialize_body(res.body) : []

edgarjs · 2021-02-03T17:29:01Z

lib/data_source/client/github.rb

+        @me_account = config[:me_account]
+        @pr_all_involve_me = config[:pr_all_involve_me]
+
+        @cache_name_hash = {
+            user_repos: 'user_repos',
+            user_orgs: 'user_orgs',
+            user_pulls: 'user_pulls'
+        }
+
+        @cache_ttl_hash = {
+            user_repos: config[:cache_ttl_sec_repo],
+            user_orgs: config[:cache_ttl_sec_org],
+            user_pulls: config[:cache_ttl_sec_pr]
+        }


Now that we have more options in the configuration, let's only assign the config hash as an instance variable and access all the individual key from it.

def initialize(config) @config = config end

Another refactor for later could be moving all the configuration to its own class in DataSource::ClientConfiguration

edgarjs · 2021-02-03T17:34:53Z

lib/data_source/client/github.rb

+        all_user_repos = Array.new
+        responses.each do |response|
+          all_user_repos = all_user_repos + response
        end
+        all_user_repos


I think this is iterating unnecessarily. You can return responses directly. And even remove it all together since with_cache is the last call in the method (But also remove the assignation of the variable if it's not used (responses = ...))

Suggested change

all_user_repos = Array.new

responses.each do |response|

all_user_repos = all_user_repos + response

end

all_user_repos

actually no, because responses is an array of response, each element is a response for a page.

The iterating here is to concatenate the elements inside each response together.

But it's just copying them over another array isn't it? all_user_repos is also an array, right?

It’s more like flatting:
From [[a],[b],[c]] to [a, b ,c]

I see. In that case you can use Ruby's Array.flat_map method:

responses.flat_map

yep, I have updated this in my previous commit. this comment is outdated.

edgarjs · 2021-02-03T17:36:18Z

lib/data_source/client/github.rb

+        all_user_pulls = Array.new
+        responses.each do |response|
+          all_user_pulls = all_user_pulls + response[:items]
        end
-        response[:items]
+        all_user_pulls


You can also simplify this with flat_map:

responses.flat_map { |r| r[:items] }

edgarjs · 2021-02-03T17:39:25Z

lib/data_source/client/github.rb

-        path = File.join(@cache_dir, filename)
-        File.open(path, 'w') { |f| f.write(value) }
+        path = File.join(@cache_dir, @cache_name_hash[filename])
+        File.open(path, 'w') { |f| f.write(JSON.dump(value)) }


I originally passed the response.body directly to avoid parsing twice. Do you think there could be another way to keep that and at the same time add support for your pagination changes? That'd be neat!

I did think about that, but since the cache is going to hold multiple responses, the neatest way is to leverage some structured data (i.e.JSON).

edgarjs · 2021-02-03T17:43:28Z

app.rb

+          cache_ttl_sec_repo: (ENV['CACHE_TTL_SEC_REPO'] || (24 * 60 * 60)).to_i,
+          cache_ttl_sec_org: (ENV['CACHE_TTL_SEC_ORG'] || (24 * 60 * 60)).to_i,
+          cache_ttl_sec_pr: (ENV['CACHE_TTL_SEC_PR'] || (5 * 60)).to_i,
+          )


See me other comment related to the configuration hash below and maybe we can move the defaults there as well.

Co-authored-by: Edgar <edgar.js@gmail.com>

edgarjs

I think you may want to setup an editorconfig plugin for your editor. So it automatically configures to follow conventions for indentation spaces and such.

app.rb

lib/data_source/client/github.rb

edgarjs · 2021-02-11T00:46:24Z

lib/data_source/client/github.rb

+        all_user_repos = Array.new
+        responses.each do |response|
+          all_user_repos = all_user_repos + response
        end
+        all_user_repos


But it's just copying them over another array isn't it? all_user_repos is also an array, right?

Co-authored-by: Edgar <edgar.js@gmail.com>

ileodo · 2021-02-13T11:05:44Z

@edgarjs any more comment I can help with?

edgarjs · 2021-02-14T16:53:44Z

Thank you for your changes @ileodo. They have been merged and published in 3.1.0

ileodo force-pushed the working branch from 2510d71 to e79edec Compare January 31, 2021 04:01

Improvement

7644b2d

ileodo force-pushed the working branch from e79edec to 7644b2d Compare January 31, 2021 04:04

ileodo commented Jan 31, 2021

View reviewed changes

ileodo mentioned this pull request Jan 31, 2021

Projects within organizations are not shown #28

Open

edgarjs reviewed Feb 1, 2021

View reviewed changes

ileodo added 3 commits February 1, 2021 22:47

fix indent

1e5e7c2

Added: unit test

4f6dcac

Fixed: pr_all_involve_me setting

bc3c80e

edgarjs reviewed Feb 3, 2021

View reviewed changes

ileodo and others added 3 commits February 4, 2021 01:35

Update lib/data_source/client/github.rb

c858791

Co-authored-by: Edgar <edgar.js@gmail.com>

Update lib/data_source/client/github.rb

c8b8a2e

Co-authored-by: Edgar <edgar.js@gmail.com>

code review

d03f1f3

edgarjs reviewed Feb 11, 2021

View reviewed changes

ileodo and others added 5 commits February 11, 2021 16:31

Update app.rb

e4c55e4

Co-authored-by: Edgar <edgar.js@gmail.com>

Update lib/data_source/client/github.rb

cd41265

Co-authored-by: Edgar <edgar.js@gmail.com>

Update lib/data_source/client/github.rb

869a0d2

Co-authored-by: Edgar <edgar.js@gmail.com>

Update app.rb

b984987

Co-authored-by: Edgar <edgar.js@gmail.com>

Update lib/data_source/client/github.rb

295d7bf

Co-authored-by: Edgar <edgar.js@gmail.com>

edgarjs approved these changes Feb 14, 2021

View reviewed changes

edgarjs merged commit ff9c0fc into edgarjs:master Feb 14, 2021

This pull request was closed.

Some Improvements #32

Some Improvements #32

Conversation

ileodo commented Jan 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgarjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgarjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ileodo Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgarjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ileodo commented Feb 13, 2021

edgarjs commented Feb 14, 2021

ileodo Feb 4, 2021 •

edited

Loading