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

Update class methods to be instance methods. #3173

Merged
merged 1 commit into from
Apr 15, 2019

Conversation

jacquesporveau
Copy link
Contributor

Description
There are no class methods defined on Spree::Order with the names the docs suggest. These are supposed to be instance methods.

There are no class methods defined on `Spree::Order` with the names the docs suggest. These are supposed to be instance methods.
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks! I just left a question, let me know what do you think.

@@ -137,20 +137,20 @@ If you want to retrieve the line item adjustments, you can use the
`line_item_adjustments` method:

```ruby
Spree::Order.line_item_adjustments
Spree::Order.find(1).line_item_adjustments
Copy link
Member

Choose a reason for hiding this comment

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

What about using order.line_item_adjustments. I think it should be pretty clear that it refers to an instance of the Spree::Order` class. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going by the how the rest of the file is formatted I think we should take this route.

The adjustment scopes section is all formatted like this.

- `Spree::Order.find(1).adjustments.eligible`: Returns all of the eligible
  adjustments on the order with ID `1`.
- `Spree::LineItem.find(1).adjustments.eligible`: Returns all of the eligible
  adjustments on the line item with ID `1`.
- `Spree::Shipment.find(1).adjustments.eligible`: Returns all of the eligible
  adjustments on the shipment with ID `1`.

as well as part of the other adjustments docs.

Spree::Order.find(1).adjustments

That makes me imagine that this is probably a pattern found throughout these docs and for the sake of consistency we should follow it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we should be consistent as well, thanks for pointing that out!

Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

Thanks for the doc love! 👍

@jacobherrington jacobherrington merged commit e2d41f2 into solidusio:master Apr 15, 2019
@jacquesporveau jacquesporveau deleted the patch-1 branch April 15, 2019 21:31
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.

3 participants