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

Put locales into config-files (yaml) #124

Closed
axsapronov opened this issue Sep 21, 2015 · 17 comments
Closed

Put locales into config-files (yaml) #124

axsapronov opened this issue Sep 21, 2015 · 17 comments

Comments

@axsapronov
Copy link
Collaborator

I think locales in .py is bad practice, because locale is class with ONE method (__init__).

My suggestion: put locales into yaml-files.

@bear
Copy link
Owner

bear commented Sep 21, 2015

would locales then become parameters to the main call?
Can you work up what this would look like as a PR?

@axsapronov
Copy link
Collaborator Author

Ok, I write this code

@axsapronov
Copy link
Collaborator Author

I tried. There is no result. I will still try

@bear
Copy link
Owner

bear commented Oct 16, 2015

no worries - I appreciate you wanting to solve this

@philiptzou
Copy link
Collaborator

The yaml files will introduce new dependency to PyYAML. I don't like the new dependency (which means you had to compile PyYAML before pdt get installed).

@bear
Copy link
Owner

bear commented Oct 16, 2015

my thought about that would be to take the work of converting it to YAML and then see if that could be done as JSON.

another option would be to make the PyYAML part only something that would have to be done during the build process -- i.e. generate the data that would be used during normal pdt use by reading and processing the PyYAML

@axsapronov
Copy link
Collaborator Author

You can find my code in branch yaml_locales

@bear
Copy link
Owner

bear commented Oct 16, 2015

thanks @warmonger1 !

@axsapronov
Copy link
Collaborator Author

@bear, in branch yaml_locales - many yaml files, but not all tests pass

@philiptzou
Copy link
Collaborator

@warmonger1 I agreed with your opinion that .py with class is bad practice. But whether YAML or JSON they all share two disadvantages that are: the unserialization reduces performance, also the flexibility of YAML || JSON are both lower than .py files. So I'm thinking about other pure Python built-in solutions. Especially a few of .py files like Django settings.py but not using class. I'll write another version about this thought and let's discuss on these two solutions.

@bear
Copy link
Owner

bear commented Oct 18, 2015

@philiptzou - looking forward to a .py solution that covers the concern that @warmonger1 raised

@philiptzou
Copy link
Collaborator

I did the modification in #130 which based on @warmonger1's changes.

yaml_locales...philiptzou:alt_locales

@axsapronov
Copy link
Collaborator Author

Sorry, now i have many works :( I can't create full pull request with this feature.
We can base on PR #130 by @philiptzou

@philiptzou
Copy link
Collaborator

@warmonger1 you can push it to your folked repository and you can create pull-request. It's a bit easier for us to review every lines you changed. Feel free to modify anything based on #130.

@bear
Copy link
Owner

bear commented Nov 5, 2015

edited: removed blocking comment

@philiptzou
Copy link
Collaborator

@warmonger1 Will you add new changes for this issue? Or this issue is solved and can be closed?

@bear
Copy link
Owner

bear commented Nov 5, 2015

oh right, this may not be blocking v2 anymore - i'll remove it for now and we can add it back if needed

@bear bear closed this as completed Nov 5, 2015
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

No branches or pull requests

3 participants