hmpf / easydmp

MIT License
7 stars 2 forks source link

Fix bug due to url arg now being int, not str #169

Closed hmpf closed 3 years ago

hmpf commented 3 years ago

The path() function can cast to a type, and doesn't send in just strings. This confused plan.NewQuestionView.get_initial(), which assumed it would get a string.

Also, plan.NewQuestionView.get_initial() now behaves more like UpdateLinearSectionView.get_initial_for_answer(), both now using AnswerHelper.

codecov[bot] commented 3 years ago

Codecov Report

Merging #169 (9f575b4) into master (bc8448a) will decrease coverage by 0.01%. The diff coverage is 12.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
- Coverage   55.15%   55.14%   -0.02%     
==========================================
  Files         102      102              
  Lines        5898     5901       +3     
==========================================
+ Hits         3253     3254       +1     
- Misses       2645     2647       +2     
Impacted Files Coverage Δ
src/easydmp/plan/views/__init__.py 32.24% <0.00%> (ø)
src/easydmp/plan/models.py 42.02% <33.33%> (-0.06%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bc8448a...9f575b4. Read the comment docs.

vbhavdal commented 3 years ago

So the problem is the type of self.question_pk? I guess the log lines further down using %s is OK?

hmpf commented 3 years ago

So the problem is the type of self.question_pk?

Yes. Often, whether it is a string or an int doesn't matter to Django, but in the json blobs the question pks are always turned into strings.

I guess the log lines further down using %s is OK?

That was the only supported format once upon a time. We could have a dugnad changing over to the new style perhaps :) EasyDMP started on Python 2.6 or so after all.

hmpf commented 3 years ago

Hm, I still can't see that f-strings are officially supported in logs. Newer Djangos though, have a trick in the logging setup. Here's from django.utils.log.DEFAULT_LOGGING on Django 3.1.x:

     'formatters': {
         'django.server': {
             '()': 'django.utils.log.ServerFormatter',
             'format': '[{server_time}] {message}',
             'style': '{',
         }
     },

I presume the "style"-flag allows for f-strings? More research needed.

hmpf commented 3 years ago

Switching away from '%'-logs is clunky but doable: https://docs.python.org/3/howto/logging-cookbook.html#formatting-styles