Skip to content

Conversation

@slad2019
Copy link

I have implemented a Phugpa version of Tibetan calendar, tested with many different days, and want to contribute to convertdate package instead of a standalone one.

@fitnr
Copy link
Owner

fitnr commented Jan 29, 2022

Thanks for the contribution. I think this would make an excellent addition to the library. I especially appreciate the details docstring.

I see that there's a class (Cal_school) that's being used once to compute the constants used in calculation. Is there a need for this to be a class? Could it be a function that returns the constants, or could we just make the constants global variables, which is a pattern used in other modules?

Can you also add a tibetan.rst file to docs/modules, following the pattern of the other libraries?

@slad2019
Copy link
Author

slad2019 commented Feb 2, 2022 via email

@slad2019
Copy link
Author

slad2019 commented Feb 6, 2022

I have added docs/tibetan.rst, please merge the latest one.

@slad2019
Copy link
Author

any question?

move import unittest before import convertdate per pylint.
@slad2019
Copy link
Author

fixed one pylint warning.

Comment on lines +153 to +154
def to_jd(year, month, leap_month, day, leap_day):
'''Obtain Julian day for Tibetan date'''
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the leap_month and leap_day arguments necessary here? Shouldn't the converter be able to check if the given month/day is leap?

@fitnr
Copy link
Owner

fitnr commented Mar 26, 2022

I've pushed a commit (in a new branch) that refactors this slightly: 0a16a9b

My main goal was to set the library up for future expansion without other changes. I added a method option to many function, which follows the pattern in other converters. So in effect one is calling this: tibetan.from_gregorian(2022, 3, 26, method=tibetan.PHUGPA)

To support this, I reduced the class to a dictionary (with a few other constants). Having all the constants available seems cleaner to me than using a class.

Let me know what you think.

@slad2019
Copy link
Author

slad2019 commented Mar 27, 2022 via email

@slad2019
Copy link
Author

slad2019 commented Mar 29, 2022 via email

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

Successfully merging this pull request may close these issues.

3 participants