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

Some general feedback #1

Open
Qqwy opened this issue Jun 28, 2017 · 2 comments
Open

Some general feedback #1

Qqwy opened this issue Jun 28, 2017 · 2 comments

Comments

@Qqwy
Copy link

Qqwy commented Jun 28, 2017

Here are some notes, after having glanced over the library during breakfast.

  • Calendrical:

    • The naming of days/1 for a function that returns the number of what day 'monday' is feels counter-intuitive to me. Maybe weekday_number/1 or something similar is better. Besides, the names of the weekdays (and even if there are seven days inside) depend on the actual calendar and language used, so maybe this should better be moved to another module (Such as Kday).
    • Probably, the datetime-geared functions should be removed. %DateTime{} stores the timezone offset on top of what NaiveDateTime does. At least Elixir's standard library takes the approach of having developers explicitly think about this timezone management (and thus always having the explicit NaiveDateTime -> DateTime conversion step).
      • For this library it is unfortunate that some of the RataDie-calculations inside Elixir's standard library are managed in private functions. Maybe an alternate idea would be to create a %RataDie{} calendar implementation (with trivial naive_datetime_to_rata_die and naive_datetime_from_rata_die-implementations).
  • Calendrical.Math:

    • At least the angles_to_radians functionality is not required by any calendars, as far as I know (or is it? 😁 )
    • It is worth noting that Integer (courtesy of the Calendar additions with the Rata Die day fraction calculations) now contains a gcd/2.
  • Calendrical.JulianDay:

    • line 58 contains a magic number. Probably better to create a constant for the middle of the day.
    • I don't actually think there is a reason to work with microsecond-precision day fractions here. You could just use the lowest fraction that allows you to express something. For noon, this would be {1, 2}.
    • Line 84 might convert the result to a float, which is probably not what you want.
    • Why is Calendrical.JulianDay not a Calendar behaviour implementation?
  • Calendrical.Kday:

    • Seems fine, although it is been a while since I read the K-day part of the Calendrical Calculations paper. 😅

Thank you for writing this! ❤️

@kipcole9
Copy link
Owner

kipcole9 commented Jun 28, 2017 via email

@kipcole9
Copy link
Owner

Thanks for the generous feedback.

  1. Have taken out the DateTime functions as you suggest - makes good sense to leave the reasoning to the programmer.

  2. Renamed days/1 to day_number/1 and moved it into Calendrical.Kday. I realise that not all calendars have 7 days in a week (Balinese, Chinese, ..) but a lot do. I'm trying to strike a balance so that when I implement some of the additional calendars I don't have to duplicate this reasoning everywhere. But I can defer that until I actually need to do something.

  3. The Calendrical.Math module is to support not only the Arithmetic calendars (which don't need angles_to_radians) and Astronomical calendars which do. Of course they need a whole lot of Astro calculations which I have been working on in the background. It makes my head spin but its great fun. And good catch on gcd, I'll use the Elixir one since Calendrical is dependent on Elixir 1.5 anyway.

I'll revisit Julian Day - a Calendrical behaviour makes sense unless you think its worth building our as a PR for the Calendar module.

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

No branches or pull requests

2 participants