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

Add iteration feature #4

Closed
wants to merge 5 commits into from
Closed

Conversation

rivol
Copy link

@rivol rivol commented Nov 5, 2016

AddedExportedSettings.items() that can be used to iterate over all exported settings.
Iteration abilities are useful if you want to expose all the exported settings in your template.

Also ported the demo app to Django 1.10.

This brings its behaviour closer to real dicts.
Iteration abilities are useful if you want to expose all the exported
settings in your template.
@coveralls
Copy link

coveralls commented Nov 5, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 7a18be8 on rivol:add-iteration-feature into fef7bcd on jkbrzt:master.

@jkbrzt
Copy link
Owner

jkbrzt commented Nov 5, 2016

Thanks for the pull request, @rivol!

Good point about the lack of iterability — I've just pushed a bit more robust solution based on subclassing dict (see a9ccf05).

Please feel free to open a new pull request with the some of the remaining changes from this PR (just make sure Travis is green).

@coveralls
Copy link

coveralls commented Nov 5, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling e6afc30 on rivol:add-iteration-feature into fef7bcd on jkbrzt:master.

@rivol
Copy link
Author

rivol commented Nov 5, 2016

Cool, will do. I also noticed that Travis isn't set up to test with Django >= 1.8 ATM, you might want to change that :-)

@jkbrzt
Copy link
Owner

jkbrzt commented Nov 5, 2016

@rivol great, thanks.

There's also Django without version which installs the latest version from PyPi. But sure, we could also include the ones between 1.7 and the currently latest version. Feel free to add that (you might then also need to add some of them to matrix/exclude). Cheers!

@rivol
Copy link
Author

rivol commented Nov 5, 2016

I'm trying to port my tests to work with your change, but it's problematic. I still can't use settings.items in templates due to the order of variable lookups in templates.

Django first tries dict lookup and relies on specific exceptions being raised if the key is absent from the dict (specifically one of TypeError, AttributeError, KeyError, ValueError, IndexError). django-settings-export raises UnexportedSettingError which Django treats as fatal error. See https://docs.djangoproject.com/en/1.10/ref/templates/language/#template-variables for more info.

I don't have any good ideas for solving this. Special cases could be added to __getitem__() but maybe you've got better ideas?

@jkbrzt
Copy link
Owner

jkbrzt commented Nov 5, 2016

@rivol right, didn't realize this. So let's drop the option to access the values via attributes, It's not actually even needed. These steps should do the trick:

  • Remove the __getattr__() method definition
  • Remove the attribute access mention from the docstring
  • Remove test_attribute_* tests

Thanks!

@jkbrzt
Copy link
Owner

jkbrzt commented Nov 5, 2016

…that won't actually be enough since it tries key access first, hold on

@jkbrzt
Copy link
Owner

jkbrzt commented Nov 5, 2016

The last missing step (hopefully):

  • Let the KeyError propagate if an attribute of that name exists
         try:
             return super(ExportedSettings, self).__getitem__(item)
         except KeyError:
+            if hasattr(self, item):
+                raise
             raise UnexportedSettingError(

Also fix a test that had broken after the merge.
@coveralls
Copy link

coveralls commented Nov 5, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 90b2919 on rivol:add-iteration-feature into e0e67ac on jkbrzt:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 90b2919 on rivol:add-iteration-feature into e0e67ac on jkbrzt:master.

@rivol
Copy link
Author

rivol commented Nov 5, 2016

Fails on Python 3. AFAICT hasattr() calls getattr() calls __getattr__() = __getitem__() which goes on to call hasattr() again...

@jkbrzt
Copy link
Owner

jkbrzt commented Nov 5, 2016

@rivol true, but __getattr__ should be removed (see #4 (comment)):

  • Remove the getattr() method definition

@rivol
Copy link
Author

rivol commented Nov 5, 2016

@jkbrzt nope, if I remove __getattr__ then dotted access fails, ala settings.FOO

@jkbrzt
Copy link
Owner

jkbrzt commented Nov 5, 2016

Please read the whole comment — I'm suggesting there dropping attribute access altogether :)

@rivol
Copy link
Author

rivol commented Nov 6, 2016

Oh, right, I missed that part :-P Thanks for sorting this out.
Time for 2.0 now?

@rivol rivol closed this Nov 6, 2016
@jkbrzt
Copy link
Owner

jkbrzt commented Nov 6, 2016

@rivol done, thanks for you help

https://pypi.python.org/pypi/django-settings-export

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.

None yet

3 participants