Fiscal year #5

sangamsth · 2022-05-27T06:45:45Z

No description provided.

…al-year-feature

…ntation of FiscalYear class

sarojkh

In addition to my comments below, just a general thumb of rule, the classes AdCalendar and BsCalendar should not mention or have implementations related to Fiscal Year. Remove them, and port to FiscalYear.

Kinda leaning toward the single responsibility principle. https://www.digitalocean.com/community/conceptual_articles/s-o-l-i-d-the-first-five-principles-of-object-oriented-design#single-responsibility-principle

sarojkh · 2022-05-31T07:55:06Z

lib/nepali_calendar/fiscal_year.rb

@@ -0,0 +1,25 @@
+class NepaliCalendar::FiscalYear < NepaliCalendar::Calendar


I don't see any benefit for inheriting from NepaliCalendar::Calendar. Please remove it.

sarojkh · 2022-05-31T07:58:58Z

lib/nepali_calendar/bs_calendar.rb

+      # Returns the bs date that is the beginning of the provided fiscal_year.
+      def self.beginning_of_fiscal_year_in_bs(fiscal_year)
+        start_year = fiscal_year.to_s.slice(0, 2).prepend('20')
+        start_date = NepaliCalendar::Calendar.new(nil, {year: start_year, month: 4, day: 1}) #start date of fiscal year
+      end


This should instead be handled by NepaliCalendar::FiscalYear.new(77, 78).beginning_of_year or NepaliCalendar::FiscalYear.beginning_of_year(77,78), whichever you find preferable. In the former, beginning_of_year would be an instance method, in the latter, it would be a class method.

The latter would look something like this (you might be new to this pattern).

def self.beginning_of_year(start_year, end_year) new(start_year, end_year).beginning_of_year end

sarojkh · 2022-05-31T07:59:36Z

lib/nepali_calendar/bs_calendar.rb

+      # Returns the bs date that is the end of the provided fiscal_year.
+      def self.end_of_fiscal_year_in_bs(fiscal_year)
+        end_year = fiscal_year.to_s.slice(2, 2).prepend('20')
+        end_date = NepaliCalendar::Calendar.new(nil, {year: end_year, month: 3, day: NepaliCalendar::BS[end_year.to_i][3]}) #end date of fiscal year
+      end


Similar to above.

sarojkh · 2022-05-31T08:01:46Z

lib/nepali_calendar/bs_calendar.rb

+      # Returns the ad date that is the beginning of the provided fiscal_year.
+      def self.beginning_of_fiscal_year_in_ad(fiscal_year)
+        bs_start_date= beginning_of_fiscal_year_in_bs(fiscal_year)
+        NepaliCalendar::AdCalendar.bs_to_ad(bs_start_date.year, bs_start_date.month, bs_start_date.day) #stard date in AD of the corresponding nepali date
+      end
+
+      # Returns the ad date that is the end of the provided fiscal_year.
+      def self.end_of_fiscal_year_in_ad(fiscal_year)
+        ad_end_date = end_of_fiscal_year_in_bs(fiscal_year)
+        NepaliCalendar::AdCalendar.bs_to_ad(ad_end_date.year, ad_end_date.month, ad_end_date.day)  #end date in AD of the fiscal year
+      end


Let's get rid of this method. Now that I think about it, it adds unnecessary complexity.

To achieve same, we could rather use

Get fy beginning in bs date

Convert the bs date to ad date.

sarojkh · 2022-05-31T08:03:38Z

lib/nepali_calendar/fiscal_year.rb

@@ -0,0 +1,25 @@
+class NepaliCalendar::FiscalYear < NepaliCalendar::Calendar


This class apparently also needs current_fiscal_year class method; which is currently implemented ditto-ish in two different places - bs_calendar.rb and ad_calendar.rb. Remove the existing implementations.

sarojkh

Feel free to pair with @Sagar535 if there are any confusions.

sarojkh · 2022-05-31T09:40:08Z

lib/nepali_calendar/fiscal_year.rb

+    def self.current_fiscal_year
+      current_year = NepaliCalendar::BsCalendar.ad_to_bs(Date.today.year, Date.today.month, Date.today.day)
+      if current_year.month < 4
+        fiscal_year = (current_year.year-1).to_s.slice(2,2) + current_year.year.to_s.slice(2,2)
+      else
+        fiscal_year = current_year.year.to_s.slice(2,2) + (current_year.year+1).to_s.slice(2,2)
+      end
+    end


This should return FiscalYear object.

sarojkh · 2022-05-31T09:42:59Z

lib/.byebug_history

+c
+cal.send :to_bs_date, bs_date
+bs_date = "2079-1-1"
+cal.send :to_bs_date, bs_date
+bs_date
+cal.to_bs_date bs_date
+cal = NepaliCalendar::BsCalendar.new "", year: 2079, month: 1, day: 1
+cal = NepaliCalendar::BsCalendar.new
+to_bs_date(bs_date)
+bs_date.to_bs_date
+bs_date.class
+bs_date


Please remove this file. Also, maybe update the .gitignore rules so that it's excluded from the future. Also, I wonder why this is inside your 'lib' folder.

Maybe this will help https://stackoverflow.com/questions/45096761/how-do-you-change-byebug-history-location

sarojkh · 2022-05-31T09:45:55Z

lib/nepali_calendar/bs_calendar.rb

+      # Returns the bs date that is the beginning of the provided fiscal_year.
+      def self.beginning_of_fiscal_year_in_bs(fiscal_year)
+        start_year = fiscal_year.to_s.slice(0, 2)
+        end_year = fiscal_year.to_s.slice(2,2)
+        NepaliCalendar::FiscalYear.new(start_year, end_year).beginning_of_year #start date of fiscal year
+      end
+
+
+      # Returns the bs date that is the end of the provided fiscal_year.
+      def self.end_of_fiscal_year_in_bs(fiscal_year)
+        start_year = fiscal_year.to_s.slice(0, 2)
+        end_year = fiscal_year.to_s.slice(2,2)
+        NepaliCalendar::FiscalYear.new(start_year, end_year).end_of_year
+      end
+
+      # Returns the fiscal year represented as a string in the form of 7778.
+      def self.fiscal_year_for_bs_date(bs_year, bs_month, bs_day) # (2079, 2, 12) ==> 7879
+        if bs_month < 4  #compare with start date of nepali fiscal year to determine the fiscal year
+          fiscal_year = (bs_year - 1).to_s.slice(2,2).to_s + bs_year.to_s.slice(2,2).to_s
+        else
+          fiscal_year = bs_year.to_s.slice(2,2).to_s + (bs_year + 1).to_s.slice(2,2).to_s
+        end
+      end
+
+      # [date] -> This is a Date object (and obviously represents AD date)
+      # Returns the fiscal year represented as a string in the form of 7778.
+      def self.fiscal_year_for_ad_date(date)
+        bs_date = BsCalendar.ad_to_bs(date.year.to_s, date.month.to_s, date.day.to_s)
+        if bs_date.month < 4
+          fiscal_year = ((bs_date.year - 1).to_s.slice(2,2)).to_s + bs_date.year.to_s.slice(2,2).to_s
+        else
+          fiscal_year =bs_date.year.to_s.slice(2,2).to_s + ((bs_date.year + 1).to_s.slice(2,2)).to_s
+        end
+
+      end
+
+      def self.get_fiscal_year(bs_date)
+        year = bs_date.slice(0,4).to_i
+        month = bs_date.slice(4, 2).to_i
+        day = bs_date.slice(6, 2).to_i
+        if month < 4  #compare with start date of nepali fiscal year to determine the fiscal year
+          fiscal_year = (year - 1).to_s.slice(2,2).to_s + year.to_s.slice(2,2).to_s
+        else
+          fiscal_year = year.to_s.slice(2,2) + (year + 1).to_s.slice(2,2)
+        end
+      end


Please see review description #5 (review)

sarojkh · 2022-05-31T09:46:46Z

lib/nepali_calendar/ad_calendar.rb

+    def self.get_fiscal_year(ad_date)
+      bs_date = NepaliCalendar::BsCalendar.ad_to_bs(ad_date.year.to_s, ad_date.month.to_s, ad_date.day.to_s)
+      if bs_date.month < 4
+        fiscal_year = ((ad_date.year - 1).to_s.slice(2,2)).to_s + ad_date.year.to_s.slice(2,2).to_s
+      else
+        fiscal_year =ad_date.year.to_s.slice(2,2).to_s + ((ad_date.year + 1).to_s.slice(2,2)).to_s
+      end
+    end


Please see review description #5 (review)

sarojkh · 2022-06-02T04:18:47Z

lib/nepali_calendar/fiscal_year.rb

+        end
+    end
+
+    def self.fiscal_year_for_bs_date(bs_year, bs_month, bs_day) # (2079, 2, 12) ==> 7879


Let's not separately support the passing of the year, month, and day. Instead, let's have it take one parameter, which should be the 'NepaliCalendar::Calendar' object.

sarojkh · 2022-06-02T04:21:29Z

lib/nepali_calendar/fiscal_year.rb

+    end
+
+    def self.current_fiscal_year
+      current_year = NepaliCalendar::BsCalendar.ad_to_bs(Date.today.year, Date.today.month, Date.today.day)


The wording of this variable - current_year - is not intuitive, since it is actually storing bs date (or NepaliCalendar::BsCalendar object to be precise). Rename it to bs_date_today or today_in_bs.

sarojkh · 2022-06-02T04:23:15Z

lib/nepali_calendar/fiscal_year.rb

+    #     FiscalYear.new(start_year, end_year).end_of_year
+    # end
+
+    def self.get_fiscal_year_from_ad(ad_date)


Suggested change

def self.get_fiscal_year_from_ad(ad_date)

def self.fiscal_year_for_ad_date(ad_date)

sarojkh

@sangamsth LGTM, but still needs comprehensive specs to cover new modifications (additions/deletions/adjustments)

lib/nepali_calendar/fiscal_year.rb

sarojkh · 2022-06-08T05:50:30Z

@sangamsth In addition to my review comments above, about 10+ spec examples are currently failing. The whole suite should pass. Try running rspec . in the project home directory.

cc @Sagar535

sarojkh · 2022-06-08T10:32:39Z

@sangamsth Ideally we want to re-request for a review, after the author of the PR has made necessary changes from the review, or else has put an argument against the requested changes.

Removing the review request for now. Re-request again, when applicable.

sarojkh

LGTM!

sangamsth and others added 9 commits May 26, 2022 17:20

fiscal year feature

510ed74

Merge branch 'master' of github.com:Daanphe/nepali_calendar into fisc…

a5577ec

…al-year-feature

test for fiscal year feature

5926d97

removed unwanted files

35e3a70

stubs Date to pass test for all date

fb09234

removes new lines

dbad3c1

- merged with fiscal-year-features

6cda54c

fix for fetching wrong last date of the given fiscal year and impleme…

bf0c6b8

…ntation of FiscalYear class

removed unnessary file and minor changes in fiscal_year

375fcb4

sarojkh mentioned this pull request May 31, 2022

fiscal year feature #4

Closed

sarojkh requested changes May 31, 2022

View reviewed changes

sangamsth added 2 commits May 31, 2022 15:18

changes form review

dfb0837

cleanup

c4ce0f9

sangamsth requested a review from sarojkh May 31, 2022 09:35

sarojkh requested changes May 31, 2022

View reviewed changes

sangamsth added 2 commits May 31, 2022 15:36

minor fixes

642c788

FiscalYear object and refactoring

ad58861

sangamsth requested a review from sarojkh May 31, 2022 11:35

sarojkh requested changes Jun 2, 2022

View reviewed changes

changes from review

417cf74

sangamsth requested a review from sarojkh June 3, 2022 04:23

sangamsth added 3 commits June 7, 2022 10:05

updates rails version to 6

f0c6b5c

rails version to 6.1.0

258a269

changes in fiscal year methods

b0bbac5

sarojkh reviewed Jun 7, 2022

View reviewed changes

few adjustments in specs

d716c03

sangamsth requested a review from sarojkh June 7, 2022 09:35

sarojkh requested changes Jun 8, 2022

View reviewed changes

re written test case for FiscalYear

e7f0d62

sangamsth requested a review from sarojkh June 8, 2022 09:57

more test cases for FIscalYear

d4baacb

sarojkh removed their request for review June 8, 2022 10:32

BsCalendar object instead of Calendar only

405ed8c

sangamsth requested a review from sarojkh June 10, 2022 07:39

sarojkh approved these changes Jun 10, 2022

View reviewed changes

sarojkh merged commit 51bda35 into Daanphe:master Jun 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fiscal year #5

Fiscal year #5

sangamsth commented May 27, 2022

sarojkh left a comment

sarojkh May 31, 2022

sarojkh May 31, 2022

sarojkh May 31, 2022

sarojkh May 31, 2022

sarojkh May 31, 2022

sarojkh left a comment

sarojkh May 31, 2022

sarojkh May 31, 2022

sarojkh May 31, 2022

sarojkh May 31, 2022

sarojkh May 31, 2022

sarojkh Jun 2, 2022

sarojkh Jun 2, 2022

sarojkh Jun 2, 2022 •

edited

Loading

sarojkh left a comment

sarojkh commented Jun 8, 2022 •

edited

Loading

sarojkh commented Jun 8, 2022

sarojkh left a comment

		@@ -0,0 +1,25 @@
		class NepaliCalendar::FiscalYear < NepaliCalendar::Calendar

	def self.get_fiscal_year_from_ad(ad_date)
	def self.fiscal_year_for_ad_date(ad_date)

Fiscal year #5

Fiscal year #5

Conversation

sangamsth commented May 27, 2022

sarojkh 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

sarojkh 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

sarojkh Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

sarojkh left a comment

Choose a reason for hiding this comment

sarojkh commented Jun 8, 2022 • edited Loading

sarojkh commented Jun 8, 2022

sarojkh left a comment

Choose a reason for hiding this comment

sarojkh Jun 2, 2022 •

edited

Loading

sarojkh commented Jun 8, 2022 •

edited

Loading