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

consolidation, refactoring and restructuring #3

Closed
wants to merge 42 commits into from

Conversation

nocash
Copy link

@nocash nocash commented Sep 8, 2014

Hello. I found this repo while looking for an GW2 API wrapper and did some work on it over the weekend. It's mostly consolidation, refactoring and restructuring with a few tweaks and added methods. I didn't make any major changes to the public interface, but the diff is pretty large due to the number of files touched by the refactoring. Because of this, I've included a summary of the more significant changes below. Let me know if you have any questions or feedback.


Created ApiRequest class to DRY up use of HTTPS and JSON modules and make a place for shared methods. Converted endpoint modules to classes (Item, Event, etc.) so they could extend ApiRequest. Created the ApiRequest.get method to encapsulate requests and parsing of responses to further DRY endpoint methods (Map.all, Map.floor, etc.).

Switched from using Net::HTTP directly to using OpenURI.read (a Net::HTTP wrapper) in order to simplify code and improve readability.

Consolidated API endpoint methods into respective class files (i.e. definitions for both Item.all and Item.details can be found in lib/gw2/item.rb instead of separately in lib/gw2/item/items.rb and lib/gw2/item/item_details.rb. This provides a cleaner and more complete view of each class' capabilities and responsibilities.

Consolidated specs into fewer files to maintain parity with the code and for the same reasons as above.

Created a shared example for API request/response parsing and rewrote specs to use it. All of the tests (both before and after the refactor) are basically checking the same three things: 1) that a URI was created correctly, 2) a web request was made to the URI, and 3) the returned JSON data was correctly parsed and returned. What that JSON data consists of really doesn't matter for this purpose, so the detailed (and sometimes very large) sets of test data aren't necessary. In the future it could be good to add acceptance tests using VCR to capture and test against actual response data.

Created ApiHelper spec helper and the #stub_endpoint method in order to simplify request stubs and DRY up code.

Extracted code to build API URIs into HTTPS.endpoint_uri to simplify code, improve readability and allow it to be reused in spec helpers.

Configured Rspec to automatically include files under spec/support to simplify the creation of additional helpers or shared examples.

Made various small changes to .gitignore file:

  • Added entry for coverage directory.
  • Added entry for .ruby-gemset file.
  • Removed entry for .DS_Store. This file is specific to a developer's environment and is not generated by or related to the project. It should be placed in the ~/.gitignore file instead.
  • Appended slashes to directory entries. Not strictly necessary, but removes ambiguity from entries.
  • Sorted entries alphabetically. This makes it easier to scan the file and remove duplicate entries. While not very beneficial for this particular project, I think it's a good habit.

Made various a small change to Gemfile:

  • Added pry-byebug to Gemfile to assist with development and debugging.
  • Sorted Gemfile entries alphabetically (see similar comment for .gitignore above for rationale).

Bumped version from "1.2.0" to "1.2.1". Despite a lot of refactoring there were no significant or compatibility-breaking changes to any public interface.

Also splits the test for .where (previously .events) into three distinct
tests. Stubs existed in the previous test but were never excercised.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b5c4246 on nocash:develop/consolidation-refactor into 2df0db6 on parix:master.

This stuff is *important*, people!
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling af8e53c on nocash:develop/consolidation-refactor into 2df0db6 on parix:master.

@parix
Copy link
Owner

parix commented Oct 15, 2014

This looks great. However, I am about the release a major refactor of the codebase. Since this is just a pure refactor and restructure (albiet a very good one), I am not sure how to go about this. I can merge this right now, but if there's too many merge conflicts during a merge of my refactor I most likely will just do a force push (due to my laziness).

Edit: I apologize for the very late response. I have not been very active on github lately due to more responsibilities at work.

@parix parix closed this Nov 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants