Cleanup README and docs and metadata #431

kmcphillips · 2016-12-03T22:48:05Z

Moving around documentation and getting it up to date. Removing references to Jaded Pixel. Setting expectations for contribution. Getting everything more consistent and cleaned up.

Bonus: Forgot to require pry in test.

kmcphillips · 2016-12-03T22:49:30Z

lib/active_shipping.rb

@@ -1,26 +1,3 @@
-#--
-# Copyright (c) 2009 Jaded Pixel


thegedge · 2016-12-05T15:06:30Z

README.md


-For the features that you add, you should have both unit tests and remote tests. It's probably best to start with the remote tests, and then log those requests and responses and use them as the mocks for the unit tests. You can see how this works with the USPS tests right now:
+It would also e good to familiarize yourself with [Location](https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/location.rb), [Package](https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/package.rb), and [Response](https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/response.rb).


thegedge · 2016-12-05T15:07:08Z

README.md


-For the features that you add, you should have both unit tests and remote tests. It's probably best to start with the remote tests, and then log those requests and responses and use them as the mocks for the unit tests. You can see how this works with the USPS tests right now:
+It would also e good to familiarize yourself with [Location](https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/location.rb), [Package](https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/package.rb), and [Response](https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/response.rb).


To be consistent with other links, surround classnames with backticks (Location, Package, and Response)

thegedge · 2016-12-05T15:08:06Z

README.md

-[https://github.com/Shopify/active_shipping/blob/master/test/remote/usps_test.rb](https://github.com/Shopify/active_shipping/blob/master/test/remote/usps_test.rb)
-[https://github.com/Shopify/active_shipping/blob/master/test/unit/carriers/usps_test.rb](https://github.com/Shopify/active_shipping/blob/master/test/unit/carriers/usps_test.rb)
-[https://github.com/Shopify/active_shipping/tree/master/test/fixtures/xml/usps](https://github.com/Shopify/active_shipping/tree/master/test/fixtures/xml/usps)
+It's worth looking at [`test/console.rb`](https://github.com/Shopify/active_shipping/blob/master/test/console.rb) too.


Unless we're going to give a reason as to why, I'd probably just drop this line.

Added the reason

thegedge · 2016-12-05T15:10:00Z

README.md

+2. Create your feature branch (`git checkout -b my-new-feature`)
+3. Commit your changes (`git commit -am 'Add some feature'`)
+4. Push to the branch (`git push origin my-new-feature`)
+5. Create a new Pull Request


Could we put all of this section in CONTRIBUTING.md and just link to it from here? I'm not sure what we've done for other open source projects.

thegedge · 2016-12-05T15:12:31Z

README.md


-After you've pushed your well-tested changes to your github fork, make a pull request, and we'll take it from there! For more information, see [CONTRIBUTING.md](https://github.com/Shopify/active_shipping/blob/master/CONTRIBUTING.md).
+### How to contribute


Maybe drop this section? I would assume that anyone on GitHub with a desire to contribute will likely know these steps. We could also link to some GitHub documentation that outlines all/most of these steps. What do you think?

I kinda like that it shows "yes this repo is the same as others".

thegedge · 2016-12-05T15:15:55Z

README.md

-```
+Using bundler, add to the `Gemfile`:
+
+gem 'active_shipping'


Code fence blocks, here and below?

thegedge · 2016-12-05T15:16:24Z

README.md


-The :login key passed to ```ActiveShipping::FedEx.new()``` is really the FedEx meter number, not the FedEx login.
+```
+rake test:remote


bundle exec, to be consistent with the above command?

kmcphillips · 2016-12-05T16:19:41Z

Great feedback. Updated.

jonathankwok · 2016-12-05T16:47:18Z

CONTRIBUTING.md

+* PR should explain what the feature does, and why the change exists.
+* PR should include any carrier specific documentation explaining how it works.
+* Code _must_ be tested, including both unit and remote tests where applicable.
+* Be consistent. Write clean code that follows Ruby community standards.


Should we link example documentation? https://github.com/bbatsov/ruby-style-guide is a great public resource that most people know of but having a reference is pretty good.

jonathankwok · 2016-12-05T16:52:20Z

README.md

 ```

-...or add it to your project's [Gemfile](http://bundler.io/).

 ## Sample Usage


In here (well, below), can you update the ActiveShipping::Package comments to explain it's in HWL order? Nothing indicates that and having to dig into the source code to know something as trivial as parameter ordering kinda blows. Especially since we typically expect LWH in the shipping industry.

See:

active_shipping/lib/active_shipping/package.rb

Line 219 in 5a91534

when :x, :max, :length, :long then ary[2]

I'll add it. But we may want to fix this or be more explicit/opinionated about it.

Agreed, keyword arguments could be a verbose way of doing it but even just following conventional LWH would be good.

I actually was starting some work to create a wrapper class around this so that the detail could be hidden and we could start writing something like thing.length instead of thing[index]. I should clue that up.

Added issue outlining problems: #434

mdking · 2016-12-07T13:27:06Z

👍

Cleanup README and docs and metadata

kmcphillips commented Dec 3, 2016

View reviewed changes

thegedge reviewed Dec 5, 2016

View reviewed changes

README.md

```

Using bundler, add to the `Gemfile`:

gem 'active_shipping'

Copy link

Contributor

thegedge Dec 5, 2016

Choose a reason for hiding this comment

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

Code fence blocks, here and below?

thegedge reviewed Dec 5, 2016

View reviewed changes

kmcphillips force-pushed the doc-cleanup branch from 950b221 to f3d982e Compare December 5, 2016 16:19

kmcphillips force-pushed the doc-cleanup branch from f3d982e to 5a91534 Compare December 5, 2016 16:20

thegedge approved these changes Dec 5, 2016

View reviewed changes

jonathankwok reviewed Dec 5, 2016

View reviewed changes

kmcphillips added 2 commits December 5, 2016 13:11

Clean up documentation and metadata

91bcf8c

Require pry in test

f9f0959

kmcphillips force-pushed the doc-cleanup branch from 5a91534 to f9f0959 Compare December 5, 2016 18:11

kmcphillips merged commit e8ac53f into master Dec 5, 2016

kmcphillips deleted the doc-cleanup branch December 5, 2016 18:36

kmcphillips deployed to rubygems December 7, 2016 16:24 Active

kmcphillips had a problem deploying to rubygems-1-9-stable December 15, 2016 16:18 Failure

jonathankwok had a problem deploying to rubygems-1-9-stable January 30, 2017 18:45 Failure

jonathankwok had a problem deploying to rubygems-1-9-stable January 30, 2017 18:51 Failure

kmcphillips deployed to rubygems-1-9-stable January 30, 2017 18:52 Active

maartenvg pushed a commit that referenced this pull request Nov 9, 2017

Merge pull request #431 from Shopify/doc-cleanup

91e2b16

Cleanup README and docs and metadata

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup README and docs and metadata #431

Cleanup README and docs and metadata #431

kmcphillips commented Dec 3, 2016

kmcphillips Dec 3, 2016

thegedge Dec 5, 2016

thegedge Dec 5, 2016

thegedge Dec 5, 2016

kmcphillips Dec 5, 2016

thegedge Dec 5, 2016

thegedge Dec 5, 2016

kmcphillips Dec 5, 2016

thegedge Dec 5, 2016

thegedge Dec 5, 2016

kmcphillips commented Dec 5, 2016

jonathankwok Dec 5, 2016

jonathankwok Dec 5, 2016

jonathankwok Dec 5, 2016

kmcphillips Dec 5, 2016

jonathankwok Dec 5, 2016

thegedge Dec 5, 2016

jonathankwok Dec 5, 2016

mdking commented Dec 7, 2016


		For the features that you add, you should have both unit tests and remote tests. It's probably best to start with the remote tests, and then log those requests and responses and use them as the mocks for the unit tests. You can see how this works with the USPS tests right now:
		It would also e good to familiarize yourself with [Location](https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/location.rb), [Package](https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/package.rb), and [Response](https://github.com/Shopify/active_shipping/blob/master/lib/active_shipping/response.rb).


		After you've pushed your well-tested changes to your github fork, make a pull request, and we'll take it from there! For more information, see [CONTRIBUTING.md](https://github.com/Shopify/active_shipping/blob/master/CONTRIBUTING.md).
		### How to contribute

Cleanup README and docs and metadata #431

Cleanup README and docs and metadata #431

Conversation

kmcphillips commented Dec 3, 2016

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

kmcphillips commented Dec 5, 2016

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

mdking commented Dec 7, 2016