Open aeroastro opened 1 year ago
Thank you all for the reviews ❤️ I have squashed the commits into 1 commit to be merged.
I find to hard to say whether either is 'good' or 'bad' without seeing the examples in a wider context.
@andyw8
Thank you for the review. I can show you some example code for this section.
def self.add_to_date(date, total_semesters, total_months, total_days)
if total_semesters != 0
semester_months = (
12 * (total_semesters / 2)
) + (
(date.month == 3 ? 5 : 7) * (total_semesters % 2)
) - 1
date = semester_months.months.since(date).end_of_month
end
total_days.days.since(total_months.months.since(date).end_of_month)
end
def self.add_to_date(date, total_semesters, total_months, total_days)
if total_semesters != 0
semester_months = (
12 * (total_semesters / 2)
) + (
(date.month == 3 ? 5 : 7) * (total_semesters % 2)
) - 1
- date = date.since(semester_months.months).end_of_month
+ date = semester_months.months.since(date).end_of_month
end
- (date.since(total_months.months).end_of_month).since(total_days.days)
+ total_days.days.since(total_months.months.since(date).end_of_month)
end
def planned_end_date
if fixed_planned_end_date || planned_end_date_offset.nil?
read_attribute(:planned_end_date)
else
return nil if planned_end_date_offset.nil?
return nil if previous_planned_end_date_milestone.nil?
if previous_planned_end_date_milestone.planned_end_date.nil?
return planned_end_date_offset.days.since(self.previous_planned_end_date_milestone.actual_date) if self.previous_planned_end_date_milestone.actual_date
else
return planned_end_date_offset.days.since(self.previous_planned_end_date_milestone.planned_end_date)
end
end
end
def planned_end_date
if fixed_planned_end_date || planned_end_date_offset.nil?
read_attribute(:planned_end_date)
else
return nil if planned_end_date_offset.nil?
return nil if previous_planned_end_date_milestone.nil?
if previous_planned_end_date_milestone.planned_end_date.nil?
- return self.previous_planned_end_date_milestone.actual_date.since(planned_end_date_offset.days) if self.previous_planned_end_date_milestone.actual_date
+ return planned_end_date_offset.days.since(self.previous_planned_end_date_milestone.actual_date) if self.previous_planned_end_date_milestone.actual_date
else
- return self.previous_planned_end_date_milestone.planned_end_date.since(planned_end_date_offset.days)
+ return planned_end_date_offset.days.since(self.previous_planned_end_date_milestone.planned_end_date)
end
end
end
I have just created an example.
def calculate_day_after_tomorrow(since: nil)
if since
2.days.since(since)
else
2.days.since
end
end
def calculate_day_after_tomorrow(since: nil)
if since
- since.since(2.days)
+ 2.days.since(since)
else
- Time.current.since(2.days)
+ 2.days.since
end
end
I have just created an example code
def purchase_ticket(user, ticket)
purchased_ticket = PurchasedTicket.new(user_id: user.id, ticket_id: ticket.id, purchased_at: Time.current)
purchased_ticket.expires_at = ticket.expires_in.seconds.since(purchased_ticket.purchased_at)
purchased_ticket.save!
purchased_ticket
end
def extend_expired_at(purchased_ticket, duration)
purchased_ticket.expires_at = duration.since(purchased_ticket.expires_at)
purchased_ticket.save!
purchased_ticket
end
def purchase_ticket(user, ticket)
purchased_ticket = PurchasedTicket.new(user_id: user.id, ticket_id: ticket.id, purchased_at: Time.current)
- purchased_ticket.expires_at = purchased_ticket.purchased_at.since(ticket.expires_in.seconds)
+ purchased_ticket.expires_at = ticket.expires_in.seconds.since(purchased_ticket.purchased_at)
purchased_ticket.save!
purchased_ticket
end
def extend_expired_at(purchased_ticket, duration)
- purchased_ticket.expires_at = purchased_ticket.expires_at.since(duration)
+ purchased_ticket.expires_at = duration.since(purchased_ticket.expires_at)
purchased_ticket.save!
purchased_ticket
end
For the optional argument example:
def calculate_day_after_tomorrow(since: Time.current)
2.days.since(since)
end
and 2.days.from_now
instead of the no-arg 2.days.since
call.
Otherwise, looks good.
Should we put this into a separate guideline?
@pirj
Thank you for the review and mentioning the 2.days.from_now
.
In my opinion, I would like to focus on 1.minute.since(created_at)
for style guide, and also for rubocop-rails
Afair, since is an alias of from_now. We gave an agreement not to break other guidelines in example code, even in bad if the bad part can be presented differently.
We prefer an instance of
ActiveSupport::Duration
as a receiver to calculate relative time like1.minute.after(created_at)