-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implemented Core parts Endpoint Issue #22 #34
Implemented Core parts Endpoint Issue #22 #34
Conversation
Pull Request Test Coverage Report for Build 64
💛 - Coveralls |
lib/spacex/capsules.rb
Outdated
SPACEX::Capsules.new(data) | ||
end | ||
|
||
def info(capsule_serial = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on making the other methods private
? I took a stab at it in #36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@invacuo - sounds like a good idea. I'll refactor and submit.
lib/spacex/cores.rb
Outdated
SPACEX::Cores.new(data) | ||
end | ||
|
||
def info(core_serial = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on making the other methods private
? I took a stab at it in #36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @efl7a
Thank you very much for your help on this! Awesome to see that you not only helped implementing one endpoint but adding two new endpoints.
I added some comments but since I merged the Rockets
PR first, and this generated a bunch of conflicts here, I fixed them and created a PR on your fork. (I hope you don't mind)
So once you merge it, we're good to go here!
Thanks again!
CHANGELOG.md
Outdated
@@ -1,8 +1,17 @@ | |||
### 0.0.8 (next) | |||
### 0.0.9 (next) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version 0.0.9 wasn't released yet.
CHANGELOG.md
Outdated
|
||
### 0.0.8 (2018/10/11) | ||
|
||
* Add ability to retrieve Capsules information via `Capsules.info` - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just put 2 lines here pointing out the main changes such as: "Add Cores endpoint", "Add Capsules endpoint"
README.md
Outdated
|
||
- `SPACEX::Launches.info` Retrieve all Launches | ||
- `SPACEX::Launches.latest` Retrieve latest launch | ||
### Latest Launch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this as lost when you rebased or merged the latest master.
README.md
Outdated
- [Launches](#launches) | ||
- `SPACEX::Launches.latest` | ||
- `SPACEX::Launches.info` | ||
- [Latest Launch](#latest-launch) - `SPACEX::Launches.latest` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this as lost when you rebased or merged the latest master.
@rodolfobandeira - Thanks for working with me. I haven't done a lot of collaborating, so this was a learning experience. |
Hey, I tried to tackle Issue #22. Hope it is OK.