Clean-up code in GitHub module to reduce duplication & coupling w/ Github API #3054

BenMusch · 2017-08-13T18:54:36Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew tests with your changes locally?

This PR cleans up some of the code in the GitHub module. The goals are to reduce duplication around things like building URLs, and to prevent external code from having to know about the "key:value" format of github qualifiers

BenMusch · 2017-08-13T18:57:37Z

Two questions:

I'd love to add more tests around things like searching issues, but it seems like the method for testing network interaction is to actually make the request. Issues are a bit unstable so I can't test an issue with confidence that it won't be closed, deleted, edited, etc. Is there a strong opinion of how to mock out network interactions for that situation?
Is there a tl;dr on how to set up the tests locally? brew tests runs them all, but I'd like to be able to run bundle exec rspec ... but I keep getting errors when I do that (e.g. could not locate file utils/github

reitermarkus · 2017-08-14T06:25:36Z

Nice work here!

Is there a strong opinion of how to mock out network interactions for that situation?

I think mocking the JSON response would be enough.

reitermarkus · 2017-08-14T06:29:34Z

but I'd like to be able to run bundle exec rspec ...

If you want to run single tests, you can specify e. g. --only=utils/github, or add :focus to the describe or it block, which will run it before all other tests.

MikeMcQuaid

Great work here, code is much cleaner 👏

MikeMcQuaid · 2017-08-14T08:43:33Z

Library/Homebrew/cmd/search.rb

    valid_dirnames = ["Formula", "HomebrewFormula", "Casks", "."].freeze
-    matches = GitHub.search_code("user:Homebrew", "user:caskroom", "filename:#{query}", "extension:rb")
-    [*matches].map do |match|
+    matches = GitHub.search_code(user: ["Homebrew", "caskroom"], filename: query, extension: "rb")


Perhaps should be users if it's taking an array?

That'd be kinda difficult to pull off, since #query_string would have to know how to singularize a word. I could just make it remove an s at the end of the word, or hard-code the users case, but that would make the method too specific in my opinion

MikeMcQuaid · 2017-08-14T08:43:59Z

Library/Homebrew/cask/lib/hbc/cli/search.rb

-        [*matches].map do |match|
+        matches = GitHub.search_code(user: "caskroom", path: "Casks",
+                                     filename: query, extension: "rb")
+        Array(matches).map do |match|


Is it worth instead ensuring search_code always returns an array?

MikeMcQuaid · 2017-08-14T08:44:19Z

Library/Homebrew/test/utils/github_spec.rb

+
+  describe "::query_string" do
+    it "builds a query with the given hash parameters formatted as key:value" do
+      query = subject.query_string(user: "Homebrew", repo: "Brew")


lowercase brew here

MikeMcQuaid · 2017-08-14T08:45:45Z

Library/Homebrew/utils/github.rb

+  end
+
+  def url_to(*subroutes)
+    URI.parse(File.join(API_URL, *subroutes))


Feels a little weird using File.join in this way. I'd just use String#join

~~URI::join~~

Edit: Only works with one argument. Maybe URL.join(API_URL, subroutes.join("/"))?

Agreed that this is kinda weird -- I did it so that url_to '/repos', '/Homebrew' would work, but I'm also fine if the weird-ness outweighs that benefit

MikeMcQuaid · 2017-08-14T08:49:00Z

I'd love to add more tests around things like searching issues, but it seems like the method for testing network interaction is to actually make the request. Issues are a bit unstable so I can't test an issue with confidence that it won't be closed, deleted, edited, etc. Is there a strong opinion of how to mock out network interactions for that situation?

In general it would be good to have at least one integration tests here that actually tests hitting the GitHub API. Use an old Homebrew/brew issue and we can lock it if necessary. I wouldn't worry about it changing/breaking: if it does, we can fix it then.

Other tests beyond the one integration test: yeh, as @reitermarkus said I think just stubbing the JSON response is a good call.

Is there a tl;dr on how to set up the tests locally? brew tests runs them all, but I'd like to be able to run bundle exec rspec ... but I keep getting errors when I do that (e.g. could not locate file utils/github

Let us know where you expected this information to live. I'd love to make that more obviously documented.

BenMusch · 2017-08-14T13:00:05Z

Let us know where you expected this information to live. I'd love to make that more obviously documented.

I was expecting something to be in a file like docs/Developing.md or something similar

Edit: Also, that document could contain info about how to run brew from the source repo, took me a minute or two to realize that was why the tests were running without my changes

reitermarkus · 2017-08-14T16:37:46Z

Library/Homebrew/utils/github.rb

    end
  end

  def issues_matching(query, qualifiers = {})


qualifiers = {} -> **qualifiers, while we're at it.

reitermarkus · 2017-08-14T16:41:03Z

Library/Homebrew/utils/github.rb

      prs = open_or_closed_prs
    else
-      return
+      return []


Remove the return, and conditionally assign it to prs instead.

reitermarkus · 2017-08-14T16:47:03Z

Library/Homebrew/utils/github.rb

+    search("code", **params)
  end

  def uri_escape(query)


You can also remove the check for URI.respond_to?(:encode_www_form_component) here, since we're always using Ruby >= 2.

Maybe just replace uri_escape with URI.encode_www_form_component, since we're only using it once.

reitermarkus · 2017-08-14T16:52:04Z

Library/Homebrew/utils/github.rb

+    params_list = main_params
+
+    qualifiers.each do |key, value|
+      if value.is_a? Array


Instead of these separate checks, just use [*value], and loop through all values, even if it is only one.

reitermarkus · 2017-08-14T17:04:10Z

Library/Homebrew/utils/github.rb

flat_map here, and append to params_list instead.

reitermarkus · 2017-08-14T17:07:02Z

Library/Homebrew/utils/github.rb

Inline this again, since it is only used once.

reitermarkus · 2017-08-14T17:08:07Z

Library/Homebrew/utils/github.rb

Move prs = out of the if.

prs = if … … else … end

reitermarkus · 2017-08-14T17:09:10Z

Library/Homebrew/utils/github.rb

Inline this, since it is only used once.

Fix duplication bug

BenMusch · 2017-08-17T13:26:02Z

This PR is all set from my perspective, can I get one last review?

reitermarkus · 2017-08-17T15:30:01Z

Library/Homebrew/test/utils/github_spec.rb

+  describe "::search_issues", :needs_network do
+    it "queries GitHub issues with the passed parameters" do
+      results = subject.search_issues("brew search", repo: "Homebrew/brew", author: "avetamine", is: "closed")
+      expect(results.count).to eq(1)


This will break if that user creates a new issue which will eventually be closed. Not sure if the line below should use #first or #last then.

reitermarkus · 2017-08-17T15:55:42Z

Library/Homebrew/test/utils/github_spec.rb

      results = subject.search_issues("brew search", repo: "Homebrew/brew", author: "avetamine", is: "closed")
-      expect(results.count).to eq(1)
-      expect(results.first["title"]).to eq("brew search : 422 Unprocessable Entity")
+      expect(results.count).to be > 1


expect(results).not_to be_empty

reitermarkus · 2017-08-17T18:31:31Z

Library/Homebrew/utils/github.rb

  end
+
+  def query_string(*main_params, **qualifiers)
+    params_list = main_params


Maybe just call this params.

reitermarkus

One last comment, other than that 👍!

Everything done, except pluralising named arguments (which is unnecessary).

reitermarkus · 2017-08-18T14:26:54Z

Thanks for the great cleanup, @BenMusch!

BenMusch · 2017-08-18T14:29:37Z

No problem, thanks for the thorough reviews!

BenMusch force-pushed the github-refactor branch from 668ed4f to aa211ae Compare August 13, 2017 19:03

BenMusch added 2 commits August 13, 2017 15:03

Remove duplicate url generation logic in Github module

e93ec12

Rename path_to -> url_to since it returns a url

acf46fc

BenMusch force-pushed the github-refactor branch from aa211ae to acf46fc Compare August 13, 2017 19:03

BenMusch added 3 commits August 13, 2017 16:15

Fix syntax error

91b139a

fix typo

d052f50

Fix url encoded in expected values

24da1ec

MikeMcQuaid previously requested changes Aug 14, 2017

View reviewed changes

BenMusch added 2 commits August 14, 2017 11:08

Implement PR feedback

603bdd0

Spec for issues search

68cdb55

BenMusch force-pushed the github-refactor branch from a9c650b to 68cdb55 Compare August 14, 2017 16:06

reitermarkus reviewed Aug 14, 2017

View reviewed changes

BenMusch force-pushed the github-refactor branch from 3d6f97d to 987e588 Compare August 14, 2017 16:57

reitermarkus reviewed Aug 14, 2017

View reviewed changes

Library/Homebrew/utils/github.rb Outdated

Copy link

Member

reitermarkus Aug 14, 2017

Choose a reason for hiding this comment

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

flat_map here, and append to params_list instead.

reitermarkus reviewed Aug 14, 2017

View reviewed changes

Library/Homebrew/utils/github.rb Outdated

Copy link

Member

reitermarkus Aug 14, 2017

Choose a reason for hiding this comment

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

Inline this again, since it is only used once.

reitermarkus reviewed Aug 14, 2017

View reviewed changes

Library/Homebrew/utils/github.rb Outdated

Copy link

Member

reitermarkus Aug 14, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

Move prs = out of the if.

prs = if … … else … end

reitermarkus reviewed Aug 14, 2017

View reviewed changes

Library/Homebrew/utils/github.rb Outdated

Copy link

Member

reitermarkus Aug 14, 2017

Choose a reason for hiding this comment

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

Inline this, since it is only used once.

BenMusch force-pushed the github-refactor branch from 987e588 to ce196e5 Compare August 14, 2017 18:13

Unify vocabulary in github module, remove unnecessary logic

5f8d212

Fix duplication bug

BenMusch force-pushed the github-refactor branch from ce196e5 to 5f8d212 Compare August 14, 2017 18:45

reitermarkus reviewed Aug 17, 2017

View reviewed changes

Make test more resilient

7a93638

reitermarkus reviewed Aug 17, 2017

View reviewed changes

BenMusch added 2 commits August 17, 2017 11:57

Improve rspec readability

e096836

Fix typo in spec

564a06d

reitermarkus reviewed Aug 17, 2017

View reviewed changes

reitermarkus approved these changes Aug 17, 2017

View reviewed changes

Change var name

ca05c7f

reitermarkus merged commit 3b92f69 into Homebrew:master Aug 18, 2017

BenMusch deleted the github-refactor branch August 18, 2017 14:29

commitay mentioned this pull request Aug 18, 2017

brew cask search fails with "undefined method `map' for nil:NilClass" Homebrew/homebrew-cask#37793

Closed

4 tasks

Homebrew locked and limited conversation to collaborators May 4, 2018

Uh oh!

Clean-up code in GitHub module to reduce duplication & coupling w/ Github API #3054

Clean-up code in GitHub module to reduce duplication & coupling w/ Github API #3054

Uh oh!

Conversation

BenMusch commented Aug 13, 2017

Uh oh!

BenMusch commented Aug 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reitermarkus commented Aug 14, 2017

Uh oh!

reitermarkus commented Aug 14, 2017

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reitermarkus Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid commented Aug 14, 2017

Uh oh!

BenMusch commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reitermarkus Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenMusch commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reitermarkus left a comment

Choose a reason for hiding this comment

Uh oh!

reitermarkus commented Aug 18, 2017

Uh oh!

BenMusch commented Aug 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

BenMusch commented Aug 13, 2017 •

edited

Loading

reitermarkus Aug 14, 2017 •

edited

Loading

BenMusch commented Aug 14, 2017 •

edited

Loading

reitermarkus Aug 14, 2017 •

edited

Loading