search

vasconsaurus / produce_in_season

0 stars 0 forks source link

Month method #8

Closed vasconsaurus closed 2 years ago

vasconsaurus commented 2 years ago

Started to write the month method for item season, does this make sense?

vasconsaurus commented 2 years ago

I might have misunderstood quite a few things. 😬 I thought we would need a method to associate the month_indexes with the months names, so it would be easier for us to access the names. so that was what I was trying to achieve. how do you think we should go about it?

The other changes were done before, I ended up checking out from one branch to another, so it carried the commits. What would be a best practice?

tcione commented 2 years ago

I might have misunderstood quite a few things. 😬 I thought we would need a method to associate the month_indexes with the months names, so it would be easier for us to access the names. so that was what I was trying to achieve. how do you think we should go about it?

But then why do we need to access the names? As far as I know we only need them to show to the user. Filtering can be done by using the month's index.

However, I do think my comment was a bit obtuse. I was hinting towards having more complete PR titles and descriptions. I've written about it in more detail here: https://github.com/vasconsaurus/produce_in_season/pull/23#pullrequestreview-1000220828

The other changes were done before, I ended up checking out from one branch to another, so it carried the commits. What would be a best practice?

As we've talked in WhatsApp, the better approach would be always starting from main. If something depends on work that lives in a branch, we should consider other strategies, like switching the base branch (which can be done using the "edit" button, on the right side of the PR's title).

vasconsaurus commented 2 years ago

But then why do we need to access the names? As far as I know we only need them to show to the user. Filtering can be done by using the month's index.

I thought this might help with that, but I think I might have overcomplicated things. Although I don't thing we will need it, I updated to use I18n.

As to the PR titles and descriptions, I'll be looking at your suggestions :]