Update MerchWeek and deprecate methods from MerchCalendar #16

chylauSF · 2017-08-29T02:22:09Z

Problem

MerchWeek class needs to be updated to return Fiscal Year dates, months, quarters, and etc, based on what current date it is.

Solution

MerchWeek needs to know what calendar it needs to base its computation on, either using the RetailCalendar or the StitchFixFiscalCalendar. Depending on what date the MerchWeek class consumes, it can return the appropriate information based on what calendar it receives. If the user does not specify the calendar, it defaults to the RetailCalendar.

We also saw that in the util.rb there were some methods that we needed to use for the StitchFixFiscalCalendar to use, but since the util class was built based on the RetailCalendar, we had to copy some of the methods and add it to the StitchFixFiscalCalendar. MerchCalendar was originally built for calculating dates in terms of Retail Calendar, but now we want to break that assumption and use for any calendar, fiscal, or whatever we need to add to this gem.

Things I need help on:

Should I make the RetailCalendar completely independent of using the util class?
Any sort of re-arranging of tests? Any more tests to add? Refactoring tests?
In MerchWeek.rb:13, I am not sure if I should expose the Calendar attribute
Let me know if I document the code correctly?
What other things I should write in the ReadMe?

Pre-deployment

Update the ReadMe to better reflect the current purpose of the gem
Adding more coverage based on what coveralls says
Awaiting Reviews

Post deployment

Bump gem version to 0.3.0 and release to rubygems.

Reviewers:

@jonworek @davidamcclain @sleepn247

…endar

…e it wasn't returning the correct dates for the starting months of this fiscal year

…more specs and also needs to write more documentation

…calendar class

…endar

…ns the correct dates we're looking for especially between merch_month and year

… fix fiscal calendar

coveralls · 2017-08-29T16:54:38Z

Changes Unknown when pulling 13518ce on kill-merch-calendar into ** on master**.

sleepn247 · 2017-08-29T15:42:14Z

lib/merch_calendar/merch_week.rb

      #   @return [MerchWeek] the specific merch week
-      def find(year, julian_month, week_number=nil)
+      def find(year, julian_month, week_number=nil, options={})
+        calendar = options.fetch(:calendar, RetailCalendar.new)


Since this gets initialized, why does this need to happen again?

I am not sure what you're asking here? Calendar is refers to the different calendars that we need to use for the rest of the methods. Maybe I should've put something in the documentation above to make it clearer?

oops! I read this wrong

the find operates on the class method level, so we need the duplication. Sorry about that.

I think @sleepn247 is right to be confused. I think the source of the confusion is using class << self above to wrap all class methods.

I think it's clearer to write a method like:

def self.find...`

That way it's really clear by looking at a method that it's a class method.

Ahhh okay! I can re write it!

sleepn247 · 2017-08-29T15:45:18Z

lib/merch_calendar/retail_calendar.rb

+    def merch_year_from_date(date) #Jan 22 2017
+      date_end_of_year = end_of_year(date.year)
+      date_start_of_year = start_of_year(date.year) 
+      return date.year - 1 if date < date_start_of_year


how about a case statement here?

sleepn247 · 2017-08-29T15:45:50Z

lib/merch_calendar/retail_calendar.rb

+    # @param  [Fixnum] the merch month to convert
+    # @return [Fixnum] the julian month
+    def merch_to_julian(merch_month)
+      if merch_month > 12 || merch_month <= 0


this check feels unevenly applied throughout

What are some good places to put this check in?

I'm kind of two minds about this, the "nice way" is to make sure that we have this kind of error checking through all of our public interfaces. The "wild west" way is to say, if you give me a bad month, screw you. I think that when it's unevenly applied the danger is that we expect them to perform in a "nice way."

sleepn247 · 2017-08-29T16:15:29Z

lib/merch_calendar/retail_calendar.rb

+          end_of_week: week_end, 
+          week: week_num, 
+          calendar: RetailCalendar.new
+        })


the first MerchWeek returned here has a start_of_year of 1/29/17, is that expected?

sleepn247 · 2017-08-29T16:17:08Z

lib/merch_calendar/stitch_fix_fiscal_year_calendar.rb

+    #
+    # @param [Date] the julian date to convert to its Fiscal Year
+    # @return [Fixnum] the fiscal year that the julian date falls into
+    def merch_year_from_date(date)


intentional to be named merch_year_from_date?

The purpose of this method is that it receives a date object and it returns a merch year that the date object lands in.

So for a Retail Calendar if I gave it the Date: 1/29/17 the merch_year should be 2017.

If I gave it a Date object of 1/1/18 the merch_year should be 2017

Let me know if you have a better name for it!

sleepn247 · 2017-08-29T16:32:26Z

lib/merch_calendar/stitch_fix_fiscal_year_calendar.rb

    def merch_months_in(start_date, end_date)
+      merch_months_combos = merch_month_combo_from_dates(start_date, end_date)


something feels wonky about this.

For example, merch_months_in(Date.new(2016,7,2), Date.new(2016,7,3)) returns only one month that starts on 7/3/2016

Me and Masaki, are actually not sure about what is the point of this method? Looking at older commits, and specs it seems like we need it to return the start dates of merch_months that fall into and in between the start_date and end_date.

We asked Lisa S. who was the originator of this method, and she says that this method isn't used anymore.

sleepn247 · 2017-08-29T18:39:04Z

sorry just realized I have to press end review for the comments to show up, that was from this morning before your latest commit

sleepn247 · 2017-08-29T19:42:13Z

lib/merch_calendar/merch_week.rb

-
+
+      #defaults to Retail Calendar if no other calendar is defined
+      @calendar = options.fetch(:calendar, RetailCalendar.new)


here's the initialized calendar here, referring to prev comment

Yes it's for backwards compatibility reasons. Currently we have some parts of FT that uses the Retail Calendar methods, and if they didn't have a Calendar class identified then we use the Retail Calendar class as a default.

So people who are already using the MerchWeek class and didn't identified the Calendar, its okay because their default is the retail calendar and nothing breaks.

coveralls · 2017-08-29T20:15:04Z

Changes Unknown when pulling 871db37 on kill-merch-calendar into ** on master**.

jonworek

I still need to take another pass at this, but I wanted to leave some comments about things that could/should be worked on in the meantime.

I do think your test coverage looks pretty solid.

I want to take a closer look at how you've maintained backwards compatibility before giving a thumbs up.

jonworek · 2017-08-29T20:14:34Z

lib/merch_calendar/merch_week.rb

+    #
+    # @!attribute [r] calendar
+    #   @return [Class] the calendar that determines the merch week
+    attr_reader :date, :calendar


I think these comments would be more readable if you didn't combine the attr_readers on a single line. So...

# comments about date... attr_reader :date # comments about calendar... attr_reader :calendar

jonworek · 2017-08-29T20:21:40Z

lib/merch_calendar/merch_week.rb


    class << self

      # Locates the +MerchWeek+ for a given Julian date.
      #
      # @overload from_date(String)
      #   @param [String] julian_date a julian date in the format of +YYYY-MM-DD+
+      #   @param options [Hash] options to set your calendar, if none it will default to RetailCalendar


I'm not sure the cleanest way to do this, but I'd specify what an options hash should look like.

See what they did here as a possible example:

@param [Hash] opts the options to create a message with. # @option opts [String] :subject The subject # @option opts [String] :from ('nobody') From address # @option opts [String] :to Recipient email # @option opts [String] :body ('') The email's body #

jonworek · 2017-08-29T20:25:04Z

lib/merch_calendar/merch_week.rb

      #   @return [MerchWeek] the specific merch week
-      def find(year, julian_month, week_number=nil)
+      def find(year, julian_month, week_number=nil, options={})
+        calendar = options.fetch(:calendar, RetailCalendar.new)


I think @sleepn247 is right to be confused. I think the source of the confusion is using class << self above to wrap all class methods.

I think it's clearer to write a method like:

def self.find...`

That way it's really clear by looking at a method that it's a class method.

jonworek · 2017-08-29T20:25:38Z

lib/merch_calendar/merch_week.rb

        end
      end

      # Returns the +MerchWeek+ for today's date
      #
      # @return [MerchWeek]
-      def today
-        MerchWeek.from_date Date.today
+      def today(options={})


The options hash isn't specified in the method comments.

jonworek · 2017-08-29T20:26:41Z

lib/merch_calendar/merch_week.rb

@@ -81,108 +92,97 @@ def year_week
    end

    # This returns the "merch month" number for a date
-    # Merch months are shifted by one. Month 1 is Feb
+    # Merch months are shifted by one. 


I think you could delete # Merch months are shifted by one. . The statements you added below make this invalid.

jonworek · 2017-08-29T21:02:09Z

spec/merch_calendar/retail_calendar_spec.rb

@@ -57,6 +60,35 @@
      expect(subject.end_of_quarter(2019, 4)).to eq Date.new(2020, 2, 1)
    end
  end
+
+  describe "#quarter" do
+    it "returns the correct date" do


This isn't correct. it "returns the correct quarter number" would make more sense.

jonworek · 2017-08-29T21:04:51Z

spec/merch_calendar/retail_calendar_spec.rb

+  end
+
+  describe "#season" do
+    context "returns Spring/Summer from its merch_month" do


context "for merch months in the Spring/Summer season" more accurately describes the context.

Saying "something returns something" is only valid with an it "returns...".

For all of these rspec things I'm pointing out, a good guideline is to read the test out loud to yourself. It should make sense in plain English.

jonworek · 2017-08-29T21:06:38Z

spec/merch_calendar/retail_calendar_spec.rb

+      expect(subject.merch_to_julian(10)).to eq 11
+      expect(subject.merch_to_julian(11)).to eq 12
+      expect(subject.merch_to_julian(12)).to eq 1
+      expect { subject.merch_to_julian(13) }.to raise_error ArgumentError


I'd move these to a separate, it block, personally... it "raises an error for invalid merch months"

jonworek · 2017-08-29T21:07:41Z

spec/merch_calendar/retail_calendar_spec.rb

+        weeks = subject.weeks_for_month(2018, 2)
+        expect(weeks.size).to eq 4
+      end
+      it "returns 5 weeks for a 5-week month Fiscal Year 2019 for Sept" do


Please add newlines between blocks.

jonworek · 2017-08-29T21:12:54Z

spec/merch_calendar/retail_calendar_spec.rb

+  end
+
+  describe "#weeks_for_month" do
+    context "correct number of weeks given julian month and fiscal year" do


I don't think this qualifies as a context. Contexts are used to group related specs together, usually indicating a precondition of all specs that fall under it.

ex...

describe "a login page" do context "when the user is already logged in" do it "tells the user they're already logging in" do ... it "takes the user to the 'logged in user' page" do ...

Decided to delete the context block

…organize the logic

coveralls · 2017-08-29T23:38:43Z

Changes Unknown when pulling 9a5dafc on kill-merch-calendar into ** on master**.

coveralls · 2017-08-29T23:40:30Z

Changes Unknown when pulling bbd94b3 on kill-merch-calendar into ** on master**.

chylauSF · 2017-08-29T23:40:34Z

Hi All,

First off thank you for your feedback! Please continue to re-comb through this PR again.

I think I mainly concern with these things:

merch_months_in method where I don't know if we want this method any more, since it is causing more confusion in what it is actually doing, and I don't believe it is actually being used
The comments and documentation of the gem.
The ReadMe.md, to see if it is readable and understandable.
Feel free to re-address any issues I might've looked over, also!

coveralls · 2017-08-30T00:45:20Z

Changes Unknown when pulling b7fd60d on kill-merch-calendar into ** on master**.

coveralls · 2017-08-30T00:47:23Z

Changes Unknown when pulling 77da512 on kill-merch-calendar into ** on master**.

jonworek

There are some minor things that I think are worth addressing that I mentioned in this second pass, but I think the core logic appears to be solid. Good job!

Once you've addressed those minor things, you have my 👍. Unless you have questions on anything I asked, you can feel free to merge after addressing those things.

jonworek · 2017-08-30T13:33:45Z

README.md

@@ -21,15 +21,16 @@ gem "merch_calendar"

 ## Usage

-For converting a date into a `MerchWeek` object.
+For converting a date into a `MerchWeek` object.  `MerchWeek` object can take a date can translate to what merch week the date falls into. 


object can take a date can translate

I'm not sure what you were trying to say here, but it looks in need of revising.

I ended up deleting that sentence that I added. I think the ruby text will speak for itself

jonworek · 2017-08-30T13:34:47Z

README.md

@@ -41,6 +42,9 @@ merch_week.end_of_month         # <Date: 2014-01-04>
 merch_week.start_of_year        # <Date: 2013-02-03>
 merch_week.end_of_year          # <Date: 2014-02-01>

+merch_week.calendar             # <MerchCalendar::RetailCalendar> (the calendar we're using)


I don't know that I see a benefit to exposing this on the public interface of the class, but it doesn't really hurt anything.

What should I do? Should I delete the comment?

If it were me, I probably wouldn't have added the attr_accessor to the class. But I think it's fine to keep what you have.

jonworek · 2017-08-30T13:36:14Z

README.md


 merch_week.year                 # 2013 (the merch year associated with this date)
 merch_week.month                # 12 (the julian month that the date falls in)
 merch_week.week                 # 5 (the week number within the month)
-merch_week.year_week            # 48 (the week number within the year)
+merch_week.year_week            # 48 (the week number within the retail calendar year)
 merch_week.quarter              # 2


Shouldn't this be 4, since we're talking about a week in January?

yep! good catch! 👍

jonworek · 2017-08-30T13:39:31Z

lib/merch_calendar/merch_week.rb

    end

    # The merch season this date falls under.
    # Returns a string of +Fall/Winter+ or +Spring/Summer+
-    #
+    # THIS MIGHT NEED TO CHANGE DEPENDING ON THE CALENDAR


Does this comment apply? You seem to be changing it depending on the calendar.

Deleted! 👍

jonworek · 2017-08-30T13:42:52Z

lib/merch_calendar/stitch_fix_fiscal_year_calendar.rb

+        return QUARTER_3
+      when 10,11,12
+        return QUARTER_4
+      end


It's a good practice to always have an else for your case statements. Otherwise, the behavior is ambiguous.

I generally raise an error if I trigger the else condition.

this would be consistent with the other raise logic too.

Should I type:

def quarter(merch_month) case merch_month when 1,2,3 return QUARTER_1 when 4,5,6 return QUARTER_2 when 7,8,9 return QUARTER_3 when 10,11,12 return QUARTER_4 else nil end end

OR

def quarter(merch_month) case merch_month when 1,2,3 return QUARTER_1 when 4,5,6 return QUARTER_2 when 7,8,9 return QUARTER_3 else return QUARTER_4 end end

neither :)

else raise "invalid merch month" end

jonworek · 2017-08-30T13:44:44Z

lib/merch_calendar/stitch_fix_fiscal_year_calendar.rb

+    # @param end_date [Date] the ending date
+    # @return [Array] Array of start dates of each Fiscal Month from given dates
+    def merch_months_in(start_date, end_date) 
+      merch_months_combos = merch_year_and_month_from_dates(start_date, end_date)


merch_months_combos is a collection of merch year and months?

yep, it is a collection of merch_year and merch_months, it helps with the merch_months_in in finding the start_of_month of each merch_month inside of the merch_months_combos

jonworek · 2017-08-30T13:47:53Z

spec/merch_calendar/retail_calendar_spec.rb

+  end
+
+  describe "#weeks_for_month" do
+    it "returns 4 weeks for a 4-week month Fiscal Year 2019 for Aug" do


I'm confused by the description. Why are you referring to a "fiscal year" in the tests for the retail calendar? Same in the test below.

Copy/paste error maybe?

yes it is! 👍

jonworek · 2017-08-30T13:51:34Z

spec/merch_calendar/stitch_fix_fiscal_year_calendar_spec.rb

+      it "returns 4 weeks for a 4-week month Fiscal Year 2019 for Aug" do
+        weeks = subject.weeks_for_month(2019, 8)
+        expect(weeks.size).to eq 4
+      end


Blank line needed between blocks.

coveralls · 2017-08-30T20:38:28Z

Changes Unknown when pulling 469939a on kill-merch-calendar into ** on master**.

coveralls · 2017-08-30T21:03:30Z

Changes Unknown when pulling b3c7413 on kill-merch-calendar into ** on master**.

coveralls · 2017-08-30T21:06:46Z

Changes Unknown when pulling 1365d0d on kill-merch-calendar into ** on master**.

coveralls · 2017-08-30T21:22:35Z

Changes Unknown when pulling 4d9f6aa on kill-merch-calendar into ** on master**.

chylauSF added 30 commits August 18, 2017 15:46

added the calendar parameter and tested to see if it takes in the cal…

38e86b6

…endar

added specs for method #to_s

5d227ae

adding my notes but maybe come back to this....

25d56f8

moving forward

507ad91

deleted reliance on MerchCalendar

f63305c

added a specs for julian to merch

1d61b7f

added the julian to merch method

f01e05b

redid the merch_months_in stitchfix fiscal year calendar class becaus…

dfd9a45

…e it wasn't returning the correct dates for the starting months of this fiscal year

added a merch_year from date method

d74a16a

had the MerchWeek rb lookup its month, year, and date from its Calendar

ab73f1e

finally passed all the tests for merchweek, but still needs to write …

9b21475

…more specs and also needs to write more documentation

changed fiscal year to merch_year

b6d74e0

finished documenting the public methods in the StitchFix fiscal year …

e304ec9

…calendar class

finished documenting the retail calendar

a7462d4

finished documenting the merch_week.rb

5406e31

added comments to date conversion

00dd343

added more tests to the merch_year_from_date method looking solid :)

f61650d

finished adding specs for the tricky fiscal years

5a2c9fc

added more specs for the weeks for month for a 4-5-5 quarter

9718fe3

added the describes that corresponds to the methods within retail_cal…

bc80b2d

…endar

merch_to_julian and julian_to_merch specs added

9a559ab

added specs for retail calendar, for the weeks for month

90ab38f

.find method added more specs for Fiscal Year calendar

2342f80

added another spec for a sanity check. Checking for the next year 2020

62e110b

added more expectations for the Fiscal Calendar to make sure it retur…

829b694

…ns the correct dates we're looking for especially between merch_month and year

.today specs where added with the option to add the fiscal cal

c640359

added the method season to both the Retail and fiscal calendar

c2cb972

added teh specs for season for merch_calendar, merch week, and stitch…

7a7bb6c

… fix fiscal calendar

added quarter method along with specs

1914afa

corrected the logic between the two calendars

a982882

sleepn247 reviewed Aug 29, 2017

View reviewed changes

adding more coverage

871db37

sleepn247 reviewed Aug 29, 2017

View reviewed changes

jonworek reviewed Aug 29, 2017

View reviewed changes

chylauSF added 2 commits August 29, 2017 16:26

added more corrections from masakis and jon's review still need to re…

9a5dafc

…organize the logic

replaced its with describes and context with describes

bbd94b3

chylauSF added 2 commits August 29, 2017 17:43

updated the ReadMe

b7fd60d

added more comments

77da512

jonworek suggested changes Aug 30, 2017

View reviewed changes

chylauSF added 2 commits August 30, 2017 10:48

finishing up jons comments

e7d4285

added more raises and re arrange the parameters

469939a

chylauSF added 2 commits August 30, 2017 13:41

fixing gemfile.lock

06f2c49

added specs to cover raised errirs

b3c7413

added coverage for the raises in the fiscal calendar

1365d0d

chylauSF added 2 commits August 30, 2017 14:15

testing

67b142e

didn't work

4d9f6aa

chylauSF merged commit 8d438bc into master Aug 30, 2017

jbusser deleted the kill-merch-calendar branch December 14, 2019 17:15

		def merch_months_in(start_date, end_date)
		merch_months_combos = merch_month_combo_from_dates(start_date, end_date)



		#defaults to Retail Calendar if no other calendar is defined
		@calendar = options.fetch(:calendar, RetailCalendar.new)

Update MerchWeek and deprecate methods from MerchCalendar #16

Update MerchWeek and deprecate methods from MerchCalendar #16

Conversation

chylauSF commented Aug 29, 2017 • edited Loading

Problem

Solution

Pre-deployment

Post deployment

Reviewers:

coveralls commented Aug 29, 2017 • edited Loading

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

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

sleepn247 commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 29, 2017 • edited Loading

jonworek left a 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

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

jonworek Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 29, 2017 • edited Loading

coveralls commented Aug 29, 2017 • edited Loading

chylauSF commented Aug 29, 2017 • edited Loading

coveralls commented Aug 30, 2017 • edited Loading

coveralls commented Aug 30, 2017 • edited Loading

jonworek left a comment • edited Loading

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

jonworek Aug 30, 2017 • edited Loading

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

chylauSF Aug 30, 2017 • edited Loading

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

chylauSF commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 29, 2017 •

edited

Loading

jonworek Aug 29, 2017 •

edited

Loading

coveralls commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 29, 2017 •

edited

Loading

chylauSF commented Aug 29, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

jonworek left a comment •

edited

Loading

jonworek Aug 30, 2017 •

edited

Loading

chylauSF Aug 30, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading

coveralls commented Aug 30, 2017 •

edited

Loading