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

Added ability to set custom currency choices via CURRENCY_CHOICES settings option. Closes #211 #271

Merged
merged 6 commits into from Mar 14, 2017
Merged

Added ability to set custom currency choices via CURRENCY_CHOICES settings option. Closes #211 #271

merged 6 commits into from Mar 14, 2017

Conversation

Stranger6667
Copy link
Collaborator

As was requested in #211

What do you think about this feature?

@codecov-io
Copy link

codecov-io commented Feb 25, 2017

Codecov Report

Merging #271 into master will increase coverage by 0.32%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
+ Coverage   93.99%   94.31%   +0.32%     
==========================================
  Files          17       17              
  Lines         783      792       +9     
  Branches      150      151       +1     
==========================================
+ Hits          736      747      +11     
+ Misses         34       32       -2     
  Partials       13       13
Impacted Files Coverage Δ
djmoney/settings.py 100% <100%> (ø)
djmoney/_compat.py 100% <100%> (+2.5%)

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 0b8fcfe...5ab9844. Read the comment docs.

…ork. #218 (#270)

* Refactor migrations tests, add tests for Django 1.7+ migration framework. #218

* Fix module check

* Simplify

* Add some shims

* Fix

* Fix bytecode & migrations check

* Fix

* Cleanup, run less things in subprocesses

* Add stdout checks for migrations

* More checks

* Fix lines

* Simplify
…ed`` on Python 2. Closes #272 (#273)

* Fixed ``UnicodeEncodeError`` in string representation of ``MoneyPatched`` on Python 2. Closes #272

* Fix lint
@benjaoming
Copy link
Contributor

@Stranger6667 I'm unfortunately lacking time and bandwidth for the next 2-3 weeks :/ will put it on my getting-back TODO - if you can wait..

@Stranger6667
Copy link
Collaborator Author

@benjaoming no problem :)

@benjaoming
Copy link
Contributor

Solid work! :)

'--cov', 'djmoney',
'--cov-config', 'coveragerc.ini',
)
result.stdout.fnmatch_lines(lines)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this run all tests again but with different settings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works in the following way:

  • We create a new settings file in temp directory
  • We run pytest in subprocess to avoid all possible module reloading issues, which will be the same if we will run it against the new project
  • test is performed in a regular way
  • we compare expected output in the main process

Copy link
Contributor

Choose a reason for hiding this comment

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

Still a couple of questions :)

  • Do you mean all tests?
  • Why is module reloading a requirement? Shouldn't such be done explicitly?
  • All tests are subject to "what if modules are loaded in a different order, let's load everything again in an isolated environment"

Can this be used?

https://docs.djangoproject.com/en/dev/topics/testing/tools/#overriding-settings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. No, it runs only tests defined in the temp directory, in these cases it is 1 subprocess test per test method in the class
  2. If we load djmoney.settings in first test once it will load Django's settings. In second test if we need to test another combination of settings we will need to change them in settings.py, remove djmoney/settings.pyc and load djmoney.settings again to get new values and cover all logic branches in djmoney.settings. I'm not sure if we can do it in the same way on all Django/Python combinations that we support. So, this solution is rough but isolated :)
  3. Almost, in this case the most problematic thing is that avoiding module reloading and caching issues seems to me core complicated than running every test in the isolated environment.

I'm not sure about https://docs.djangoproject.com/en/dev/topics/testing/tools/#overriding-settings, I'll try it :)

Probably running python with -B option could help. I will try to simplify tests :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see what you mean after reading this in the docs:

Finally, avoid aliasing your settings as module-level constants as override_settings() won’t work on such values since they are only evaluated the first time the module is imported.

Maybe we can use override_settings + reload(djmoney.settings) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! It works for Django 1.10 and Python 2.7. I'll check the rest and push changes
Thank you for the review :)
The initial implementation is really an overkill

CURRENCY_CHOICES = [('USD', 'USD $'), ('EUR', 'EUR €')]

SECRET_KEY = 'foobar'
''')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is tooooooo weird IMO :)

@Stranger6667 Stranger6667 merged commit 52b9820 into django-money:master Mar 14, 2017
@Stranger6667 Stranger6667 deleted the issue-211-custom-choices branch March 14, 2017 14:30
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