Fixed #19567 -- Make js i18n view as cbv. #678

Closed
wants to merge 27 commits into
from

Projects

None yet
@niwinz
Contributor
niwinz commented Jan 26, 2013

This is a initial proposal.
I think is backward compatible.
Pass all view tests with python2 and python3.

Reviews welcome.

Ticket: https://code.djangoproject.com/ticket/19567

@typeshige

For the curious, it would be helpful if it stated briefly why this is recommended.

@typeshige It seems like the recommendation originates from gunicorn. https://gunicorn-docs.readthedocs.org/en/latest/run.html?highlight=django#gunicorn-django I'm not sure why, either.

Member

It's to do with the fact Django is now WSGI compliant so gunicorn is wrapping Django's own WSGI rather than hacking around inside django as it used to need to. I think it rather substantially reduces the amount of code on both sides.

@mjtamlyn I see, thank you.

timgraham and others added some commits Jan 23, 2013
@timgraham timgraham Fixed #19610 - Added enctype note to forms topics doc.
Thanks will@ for the suggestion.
0de2645
@jbronn jbronn Fixed typo. 71c8539
@slurms @claudep slurms Fixed #17416 -- Added widgets argument to inlineformset_factory and m…
…odelformset_factory
93e79b4
@claudep claudep Fixed #19125 -- The startproject command should validate the name ear…
…lier

Thanks Łukasz Rekucki for the report and the patch.
9893fa1
@aaugustin aaugustin Minor optimization in the static serve view. 57c6617
@ramiro ramiro More i18n makemessages tests tweaks. 57d439e
@apollo13 apollo13 Fixed a typo. e2252bf
@timgraham timgraham Fixed #19639 - Updated contributing to reflect model choices best pra…
…ctices.

Thanks charettes.
eaa716a
@ramiro ramiro Fixed #6682 -- Made shell's REPL actually execute $PYTHONSTARTUP and …
…`~/.pythonrc.py`.

Also:

* Added a ``--no-startup`` option to disable this behavior. Previous
  logic to try to execute the code in charge of this funcionality was
  flawed (it only tried to do so if the user asked for ipython/bpython
  and they weren't found)
* Expand ``~`` in PYTHONSTARTUP value.

Thanks hekevintran at gmail dot com  for the report and initial patch.

Refs #3381.
1f6b2e7
@timgraham timgraham Fixed #19577 - Added HTML escaping to admin examples.
Thanks foo@ for the report and Florian Apolloner for the review.
eafc036
@claudep claudep Fixed #19665 -- Ensured proper stderr output for Command.run_from_argv
Thanks Stefan Koegl for the report and Simon Charette for the review.
b9c8bbf
@ramiro ramiro Patch by Claude for #16084. 2babab0
@ramiro ramiro Added more shortcuts to i18n docs in index page. 5b99d5a
@ramiro ramiro Revert "Patch by Claude for #16084."
This reverts commit 2babab0.
ce27fb1
@claudep claudep Moved has_changed logic from widget to form field
Refs #16612. Thanks Aymeric Augustin for the suggestion.
ebb504d
@claudep claudep Fixed #18460 -- Fixed change detection of ReadOnlyPasswordHashField
Thanks jose.sanchez et ezeep.com for the report and Vladimir Ulupov
for the initial patch.
1686e0d
@claudep claudep Used property decorators in django/forms.py 58062a6
@aaugustin aaugustin Fixed validation of email addresses when the local part contains an @.
See also BaseUserManager.normalize_email -- it uses rsplit.

Refs #4833.
424eb67
@aaugustin aaugustin Fixed #19540 -- Stopped using deprecated os.stat_float_times. cebbec9
@aaugustin aaugustin Fixed #19589 -- assertRegexpMatches is deprecated in Python 3.3. 55416e2
@claudep claudep Moved BooleanField 'required' validation in validate() d18893d
@aaugustin aaugustin Fixed #17158 -- Used a non-ambiguous representation of SQL queries
when a correct representation cannot be obtained.
6605ac3
@claudep claudep Fixed #18483 -- Marked hidden field error string for translation
Thanks Evil Clay for the report and Emil Stenstrom for the initial
patch.
962f133
@claudep claudep Fixed #16123 -- Ensured strptime receive proper string type
strptime generates an UnicodeEncodeError when using a non-ascii
unicode string on Python 2.
4b3f711
@niwinz niwinz Fixed #19567 -- Make js i18n view as cbv. 25a34b2
@KrzysiekJ
Contributor

Thanks for this pull request, it is a good step to remove that spaghetti-like javascript_catalog function, however I have some comments after looking at the code.

Regarding “packages” and “domain” parameters, I’m not convinced that the pattern „if something is a a string, make it a singleton list” is a good approach here. It also doesn’t seem to be used widely in Django, for example admin options like list_filter don’t behave like that. Besides, explicit type checking also isn’t a good pattern here; one may want to provide another iterator than a list or tuple and Django shouldn’t prevent him from doing that. Checking isinstance(packages, collections.Iterable) would be probably better.

If we have two adjacent parameters which are “string or iterable”, then both of them should have names either in singular or plural form. I think that a reasonable approach would be to provide “domains” and “packages” parameters, both of which are assumed to be an iterable (without type checking and conversion), and make javascript_catalog to be a thin wrapper (perhaps deprecated?) around I18n view which allows specifying singular-form “domain” argument to prevent backwards compatibility.

Since this change adds a new functionality, namely specifying multiple domains for javascript catalog view, then some tests should be written to check this functionality, and it should be documented.

The view name I18n probably could be a little more verbose.

@niwinz
Contributor
niwinz commented Aug 11, 2013

A lot of thanks for your review. I will consider all your suggestions!
Now I need possibly rewrite entirely the pull request because the current version of javascript_catalog uses django templates (personally, I don't like this) and it can be take some time.

@timgraham
Member

This PR needs to be reworked so it doesn't include unrelated commits (caused by when someone accidentally force pushed to (django/django)). It also needs updates as noted above and in the ticket.

@timgraham timgraham closed this Feb 8, 2014
@mattseymour
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment