Ruby Rewrite #18

murjax · 2023-09-06T14:01:57Z

Rewrite the original Ruby project (currently in this repo under old/ruby).

Improvements made:

Remove redundant uptime check ping. If we don't get a response, we just won't do anything.
Remove git library dependency. We just run git clone directly. This project no longer requires any dependencies outside of testing.
Validate username presence, account presence, and repo info presence before cloning.
Stub externals in test. This includes the API request to Github and git clone command. Supported by RSpec method stubbing.
Create an executable binary.
Arrange files in standard directories (bin, lib).

Issues for Ruby: #22, #27, #44

ruby/README.md

MichaelDimmitt · 2023-09-18T15:03:36Z

ruby/lib/clone_handler.rb

+    return unless clonable?
+
+    Kernel.system(clone_command)
+  end


@murjax
Is call a pattern common in the ruby community? It seems something more descriptive could be helpful long term. The call here is going to hit the api if possible and get all the repos.

So maybe it could be renamed clone_all_repos ?

call is clone now. This clones all the repos.

MichaelDimmitt · 2023-09-18T15:16:05Z

ruby/lib/clone_handler.rb

+
+  def clone_command
+    @clone_command ||= response_body&.map { |info| "git clone #{info['clone_url']}" }&.join(' & ')
+  end


This is complex. (It has a lot of jobs)

To understand this I had to go work backwards from where I know the api request is in the code and move from there to the next method and then to the next function.

info_uri -> response -> response_body ->
This gets me to the first step in this clone command.

I think code is supposed to read in the other direction

aka. some pseudo code
call method does

map or while loop or iterator that does:

get_repos all_pages - just leave this one as a comment instead of having a function.

get_repos_by_page_from_api

clone_repos_api_result

doing this right after each iteration step will let us start cloning repos right away instead of needing to build the whole list.

murjax · 2023-09-21T02:17:48Z

ruby/lib/clone_handler.rb

+  PER_PAGE = 30.freeze
+  BASE_URL = 'https://api.github.com'
+
+  def self.get_username


Restored the username prompt. This will only run from the clone script if the user didn't provide one in arguments.

murjax · 2023-09-21T02:19:22Z

ruby/lib/clone_handler.rb

+  end
+
+  def clone
+    validate_clonable


Validate that we have everything before cloning. If something is missing or an API request failed, it will be added to an errors list and printed.

murjax · 2023-09-21T02:20:39Z

ruby/lib/clone_handler.rb

+  end
+
+  def repo_info
+    @repo_info ||= page_count.times.map { |index| repo_info_for_page(index + 1) }.flatten.compact


Note that this collects all repo info across pages. This all occurs before clone commands are built.

murjax · 2023-09-21T02:29:12Z

@MichaelDimmitt Can you re-review for these two questions?

Does this satisfy the specification from issue Roadmap / Application Specification #44 minus stretch?
Is this readable? It's consolidated into a single class right now. There's places to split responsibility into more classes if we want. Will you be able to read it 6 months from now as as?

MichaelDimmitt · 2023-09-29T20:08:14Z

ruby/lib/clone_handler.rb

+
+  def final_command
+    @final_command ||= clone_commands.flatten.join(' & ')
+  end


I still do not like this "final_command" name
string_to_clone_all_repos_as_bash_jobs 🤣

clone_all_command? Open to easy to read ideas.

MichaelDimmitt · 2023-09-29T20:10:37Z

ruby/lib/clone_handler.rb

+    elsif user_info&.dig('public_repos').to_i == 0
+      errors.push('No repositories found at this account')
+    end
+  end


Similar to what you wanted me to do on the bash command line

why not just forward the error message sent by the server?

Can we check http_status codes instead of string matching the api message?

This checks the user first. The response here doesn't have an error. We just know to break here if the repo count is 0. In other words there's no Github server error, it's a local error.

MichaelDimmitt · 2023-09-29T20:14:08Z

ruby/lib/clone_handler.rb

+    repo_info.each do |info|
+      if info&.dig('message')&.include?('API rate limit exceeded')
+        errors.push('API rate limit exceeded')
+      end


I think we do not need this if statement here.

This is iterating through all the repo info responses. Some may have succeeded. I don't want to add an error for successes. Can you elaborate?

MichaelDimmitt · 2023-10-03T12:12:25Z

ruby/lib/clone_handler.rb

+
+  def repo_info
+    @repo_info ||= page_count.times.map do |index|
+      next if last_repo_info_request_failed


next should be break, if the api is violated do not do any more work with the api.

I tried that. The map ends up being nil. Example:

x = [1,2,3,4] y = x.map { |z| z * 2 } # => [2, 4, 6, 8] y = x.map { |z| z == 2 ? break : z * 2 } # => nil

I might try replacing this with a reduce or manual for loop so I can properly break.

murjax force-pushed the ruby_rewrite branch from f725995 to c50bd58 Compare September 9, 2023 17:31

murjax marked this pull request as ready for review September 9, 2023 17:41

MichaelDimmitt reviewed Sep 18, 2023

View reviewed changes

ruby/README.md Outdated Show resolved Hide resolved

MichaelDimmitt reviewed Sep 18, 2023

View reviewed changes

This was referenced Sep 21, 2023

27: Add pagination support (Ruby) #32

Closed

22: Put all repos in folder named after username (Ruby) #31

Closed

murjax force-pushed the ruby_rewrite branch from c50bd58 to 9913e5c Compare September 21, 2023 02:14

murjax commented Sep 21, 2023

View reviewed changes

murjax force-pushed the ruby_rewrite branch from 9913e5c to 8d0edcb Compare September 21, 2023 02:22

murjax requested a review from MichaelDimmitt September 21, 2023 02:29

MichaelDimmitt reviewed Sep 29, 2023

View reviewed changes

murjax force-pushed the ruby_rewrite branch from 8d0edcb to a585625 Compare October 2, 2023 23:41

MichaelDimmitt reviewed Oct 3, 2023

View reviewed changes

MichaelDimmitt approved these changes Oct 3, 2023

View reviewed changes

Rewrite the project (Ruby)

f75b941

murjax force-pushed the ruby_rewrite branch from a585625 to f75b941 Compare October 3, 2023 22:55

murjax merged commit d194552 into master Oct 3, 2023

murjax deleted the ruby_rewrite branch October 3, 2023 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ruby Rewrite #18

Ruby Rewrite #18

murjax commented Sep 6, 2023 •

edited

Loading

MichaelDimmitt Sep 18, 2023 •

edited

Loading

murjax Sep 21, 2023

MichaelDimmitt Sep 18, 2023 •

edited

Loading

murjax Sep 21, 2023

murjax Sep 21, 2023

murjax Sep 21, 2023

murjax commented Sep 21, 2023

MichaelDimmitt Sep 29, 2023

murjax Sep 29, 2023

MichaelDimmitt Sep 29, 2023

MichaelDimmitt Sep 29, 2023

murjax Sep 29, 2023

MichaelDimmitt Sep 29, 2023

murjax Sep 29, 2023

MichaelDimmitt Oct 3, 2023 •

edited

Loading

murjax Oct 3, 2023

Ruby Rewrite #18

Ruby Rewrite #18

Conversation

murjax commented Sep 6, 2023 • edited Loading

MichaelDimmitt Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelDimmitt Sep 18, 2023 • edited Loading

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

murjax commented Sep 21, 2023

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

MichaelDimmitt Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murjax commented Sep 6, 2023 •

edited

Loading

MichaelDimmitt Sep 18, 2023 •

edited

Loading

MichaelDimmitt Sep 18, 2023 •

edited

Loading

MichaelDimmitt Oct 3, 2023 •

edited

Loading