a-roomana / django-jalali-date

Jalali Date support for user interface. Easy conversion of DateTimeFiled to JalaliDateTimeField within the admin site, views, forms and templates.
MIT License
292 stars 33 forks source link

Compatibility issue with jdatetime version 5 #78

Closed aminvsf closed 1 month ago

aminvsf commented 3 months ago

After upgrading from jdatetime version 4 to 5, "to_jalali" template tag stopped working, raising the following error:

image

Traceback:

image

I'll try to find a fix for it myself if I get the time to, but for now, I just want to aware the maintainers of the problem.

aminvsf commented 1 month ago

Found the problem. The problem was originally caused by me using translatable datetime formats in my JALALI_DATE_DEFAULTS settings.

My project settings.py:

image

Non-str strftime values seem to be unsupported in jdatetime version 5.

a-roomana commented 1 month ago

Hi,

Thank you so much for identifying the problem.

If lazy strings are not working in Django 5, we can change the default settings and convert the lazy strings to regular strings.

I checked if changing the default settings to regular strings causes any issues with previous versions. In past versions, lazy strings were not helpful and were equivalent to regular strings.

What is your opinion? If you agree, please update your pull request with these changes. I will merge it immediately and release the next version.

@aminvsf

aminvsf commented 1 month ago

Hello,

it's my pleasure.

The package itself is using normal strings as strftime defaults right now. It was me who used lazy ones in my project's settings.py which caused the issue.

I also believe that the problem is actually caused by the "jdatetime" package, and not Django. I had this issue with both Django 4 and 5. After a while I figured out that the issue was generated by upgrading "jdatetime" package to version 5 and having those lazy strings as strftime defaults in my settings.py.

Converting lazy strings to normal ones in the package settings.py file can be a solution, although it's not a good one in my opinion since some methods like "to_jalali" still can accept custom strftime as an argument and if someone pass a lazy string as strftime to them, it will again raise the error since jdatetime package does not accept lazy strings as strftime anymore.

So I believe having these strings converted in the methods' body as I did in my pull request can be a better solution.

Look forward to hearing your thoughts on this.

@a-roomana

a-roomana commented 1 month ago

Ok, your message is correct.

We have two solutions:

  1. Merge it, release the new version, and hope it does not have any bugs.
  2. Add a test to it, then merge it and release the new version.

Unfortunately, we don't have any tests. I asked ChatGPT how to do it, but I'm not sure it's correct. (answer of ChatGPT)

Can you test it and add a test to your changes?

If you choose option 1, that's no problem.

@aminvsf

aminvsf commented 1 month ago

Of source the second option is my style. I didn't add tests to it at first since I was unsure about my code getting accepted.

I can write test cases for these template tags using Django built-in TestCase and then setup a GitHub CI/CD pipeline for it to run these tests with different Django and Python versions.

Is that ok?

@a-roomana

a-roomana commented 1 month ago

it's great.

@aminvsf